[ntuple] Simplify RNTupleProcessor factory methods#18608
Merged
enirolf merged 2 commits intoroot-project:masterfrom May 7, 2025
Merged
[ntuple] Simplify RNTupleProcessor factory methods#18608enirolf merged 2 commits intoroot-project:masterfrom
RNTupleProcessor factory methods#18608enirolf merged 2 commits intoroot-project:masterfrom
Conversation
Test Results 19 files 19 suites 4d 0h 9m 53s ⏱️ Results for commit 679485f. ♻️ This comment has been updated with latest results. |
hahnjo
approved these changes
May 6, 2025
Member
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
LGTM, some function documentation to update
enirolf
force-pushed
the
ntuple-processor-creation
branch
2 times, most recently
from
eb4f35b to
a8ed4ff
Compare
For each factory, instead of having an overload that takes `processorName`, now it is just an optional argument with the empty string as a default argument. This simplifies and de-bloats the interface quite significantly.
Mostly to reflect the new argument order in the factory methods. In addition, the default values in the `RNTupleJoinProcessor` ctor are redundant (these arguments are always passed by the caller), so they have been removed.
enirolf
force-pushed
the
ntuple-processor-creation
branch
from
a8ed4ff to
679485f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As discussed in #18224 (comment), having a separate overload for
processorNamecauses a lot of bloat. With this PR, instead of having an overload that takesprocessorName, now it is just an optional argument with the emptystring as a default argument. This simplifies and de-bloats the interface quite significantly.
The constructors for each
RNTupleProcessorsubclass have been refactored to reflect this new argument order, and the "burden" of inferring the processor's name has been moved from the factories to the constructors.