Correct TChain::Add handling of partially loaded chain.#22672
Conversation
Test Results 21 files 21 suites 3d 13h 11m 1s ⏱️ For more details on these failures, see this check. Results for commit b06c428. |
| #include "TSystem.h" | ||
| #include "TROOT.h" | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
If this then is executed as a macro, why this namespace?
There was a problem hiding this comment.
(the macro is a copy/paste of the user report).
There was a problem hiding this comment.
ah I see: great to have such reproducers delivered to us! Perhaps this can be streamlined before integrating.
|
|
||
| TRandom3 rng(42); | ||
| const std::string base = "chain_add_repro_files"; | ||
| gSystem->Exec(("rm -rf " + base).c_str()); |
There was a problem hiding this comment.
Maybe something xplatform would be better here?
| OUTREF execTChainCleanup.ref) | ||
|
|
||
| ROOTTEST_ADD_TEST(TChainAddOtherChainGetEntries | ||
| MACRO chain_add_getentries.C+) |
There was a problem hiding this comment.
The aclic'd macro works great. I wonder if an executable can be better, making testing a bit faster, of course if no dictionary is required to be generated on the fly.
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
Thanks for the fix! I have a question and then some suggestions for the test
| for (int f = 0; f < nfiles; ++f) { | ||
| const std::string fname = dir + "/f_" + std::to_string(f) + ".root"; | ||
| TFile out(fname.c_str(), "RECREATE"); | ||
| TTree t("tree", "tree"); |
There was a problem hiding this comment.
Just adding here that time and again we've had issues with tests opening a TFile and creating a TTree on the stack. Maybe to avoid headeaches better to do both on the heap.
|
|
||
| TChain* make_chain(const std::string& dir, int nfiles) | ||
| { | ||
| TChain* c = new TChain("tree"); |
There was a problem hiding this comment.
There are many places in the test with a bare new without a corresponding delete, it would be much better to handle these via unique_ptr
| while (a->LoadTree(nav) >= 0) ++nav; | ||
| printf(" reachable via LoadTree loop = %lld ← correct total\n", nav); | ||
|
|
||
| // Workaround: add the files into a single chain instead of chaining chains. |
There was a problem hiding this comment.
Maybe I'm not following the logic well, but why is the workaround part of the test?
There was a problem hiding this comment.
I suppose it is no longer strictly necessary but harmless to do the additional cross check. I will update the comment.
| Long64_t nentries = element->GetEntries(); | ||
| if (fTreeOffset[fNtrees] == TTree::kMaxEntries) { | ||
| if (nentries == TTree::kMaxEntries) { | ||
| fEntries = TTree::kMaxEntries; |
There was a problem hiding this comment.
I don't understand this line completely. fEntries is the member of this chain (the one where Add(TChain *other) is being called). So why are we setting the number of entries to kMaxEntries if the entries of the other chain are not known? Aren't we losing info on the preexisting number of entries in this chain?
There was a problem hiding this comment.
We are not. fEntries == TTree::kMaxEntries indicates that the total number of entries is not known, which is now the case since we don't know the number of element of one of the element of the chain.
There was a problem hiding this comment.
Thanks for the fix!
I think the test should be much simplified and possibly turn into a unit test. The problem can be reproduced, e.g., with a single tree with a single entry that is first added to two chains individually, and then one is merged into the other.
4 participants
This fixes #22671.