[core] Make TDirectory::Append tolerant to identical objects. #20888
Conversation
Test Results 22 files 22 suites 3d 22h 24m 59s ⏱️ Results for commit bea103b. ♻️ This comment has been updated with latest results. |
hageboeck
force-pushed
the
root7-ownership-1
branch
2 times, most recently
from
aa1d14c to
0728d2c
Compare
hageboeck
force-pushed
the
root7-ownership-1
branch
2 times, most recently
from
7b10767 to
ae2f59b
Compare
hageboeck
force-pushed
the
root7-ownership-1
branch
2 times, most recently
from
63070c2 to
b04a01d
Compare
core/base/src/TDirectory.cxx
Outdated
| { | ||
| if (!obj || !fList) return; | ||
|
|
||
| unsigned int nExistingInstances = 0; |
There was a problem hiding this comment.
This number needs to be per instance. I.e.:
auto ptr1 = new TNamed("abc", "abc");
auto ptr2 = new TNamed("abc", "abc");
...
list->Append(ptr1, false);
list->Append(ptr1, false);
list->Append(ptr2, false);
list->Append(ptr2, true);
If I am not mistaken the code as of commit af54f14 would only remove 'one' of the ptr1.
There was a problem hiding this comment.
I updated the test, and it actually removes all instances. That's because they get pushed multiple times into the list of things to be removed.
There was a problem hiding this comment.
Your comment did make me review the counter, though, and it's not needed. Now, all duplicates are removed using the toRemoveList.
hageboeck
force-pushed
the
root7-ownership-1
branch
from
b04a01d to
c16e60f
Compare
When calling TDirectory::Append(obj, replace=true), and obj is already in the directory, don't issue a warning. Instead, proceed to clear the list of any duplicates until only one instance is left (this is the documented behaviour). If a physically different object of the same name is present, issue a warning for each of them and remove them from the list. This will enable workflows such as: auto h = new TH1D(...); dir->Append(h, true); After this change, the above lines work both with implicit object ownership off and on, resulting in only one instance in the directory. Furthermore, this fixes a bug where the following code would unset the directory of the histogram: auto h = new TH1X(...); dir->Append(h); dir->Append(h, /*replace=*/true); h->GetDirectory() == nullptr; Finally, the warning message is made a bit more accurate. If an object with the same name but a different type is being replaced, the old message would still read: "Replacing existing <Type of replacing object>" Now, the type of the replaced object is used instead of the type that replaces it.
hageboeck
force-pushed
the
root7-ownership-1
branch
from
c16e60f to
4eb7c32
Compare
hageboeck
force-pushed
the
root7-ownership-1
branch
from
4eb7c32 to
bea103b
Compare
| for (auto toRemove : toRemoveList) { | ||
| ROOT::DirAutoAdd_t func = toRemove->IsA()->GetDirectoryAutoAdd(); | ||
| if (func && toRemove != obj) { | ||
| func(toRemove, nullptr); |
There was a problem hiding this comment.
Last problem (which I don't think we can fix) is that if an histogram (that is not obj but has the same name as obj) is both in the current directory and is attached to a completely distinct TDirectory, this will result in that second histogram being removed from the other directory but not this one ....
On the other hand this would mean the user did:
TFile f1(...);
auto h1 = new TH1F("histo", ....);
TFile f2(...);
auto h2 = new TH1F("histo", ....);
f2.Append(h1);
f2.Append(h2, true);
If I read the code correctly, this would result in f1 being empty and f2 containing both histograms.
There was a problem hiding this comment.
Also related, if TH1::AddDirectoryStatus() is false, the function is a no-op :(
i.e. this 'fails':
TH1::AddDirectory(false);
TFile f1(...);
auto h1 = new TH1F("histo", ....);
h1.SetDirectory(&f1);
auto h2 = new TH1F("histo", ....);
f1.Append(h2, true);
|
Why is the proposed addition to the user code: and not ? |
3 participants
When calling TDirectory::Append(obj, replace=true), and obj is already in the directory, return without doing anything.
If obj has the same name as an existing object, but is physically different, the usual warning is raised.
This will enable workflows such as:
After this change, the above lines work both with and without auto registration to gDirectory. This is needed to prepare ROOT for working without auto registration.