Skip to content

[ntuple] Remove unused RNTupleJoinTableMethods#21468

Open
enirolf wants to merge 2 commits intoroot-project:masterfrom
enirolf:ntuple-join-table-unused-methods
Open

[ntuple] Remove unused RNTupleJoinTableMethods#21468
enirolf wants to merge 2 commits intoroot-project:masterfrom
enirolf:ntuple-join-table-unused-methods

Conversation

@enirolf
Copy link
Contributor

@enirolf enirolf commented Mar 3, 2026

These methods currently are not used in practice, or foreseen to be used. They can be re-added if the need arises.

enirolf added 2 commits March 3, 2026 15:27
These methods currently have no practical use. They can be re-added if the need for them arises again.
The TODO on line 138 of `RNTupleJoinTable.cxx` will be addressed in a
follow-up PR.
@enirolf enirolf self-assigned this Mar 3, 2026
@enirolf enirolf requested a review from jblomer as a code owner March 3, 2026 14:38
@github-actions
Copy link

github-actions bot commented Mar 3, 2026

Test Results

    22 files      22 suites   3d 4h 59m 59s ⏱️
 3 808 tests  3 806 ✅  1 💤 1 ❌
75 702 runs  75 691 ✅ 10 💤 1 ❌

For more details on these failures, see this check.

Results for commit 697333f.

♻️ This comment has been updated with latest results.

ROOT::NTupleSize_t
ROOT::Experimental::Internal::RNTupleJoinTable::GetEntryIndex(const std::vector<void *> &valuePtrs) const
ROOT::NTupleSize_t ROOT::Experimental::Internal::RNTupleJoinTable::GetEntryIndex(const std::vector<void *> &valuePtrs,
bool throwOnMultipleMatches) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this bool param a bit odd: can't the function always return the first index + e.g. a bool telling whether multiple matches exist? At that point the caller can decide if they want to throw or not.
If throwing on multiple matches was a common enough pattern then I'd see the point of this, but it seems that the caller almost never wants to throw..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants