Prepare roottest for ROOT 7's ownership model#21225
Prepare roottest for ROOT 7's ownership model#21225hageboeck wants to merge 6 commits intoroot-project:masterfrom
Conversation
Test Results 22 files 22 suites 3d 9h 36m 34s ⏱️ Results for commit 5fe91b4. ♻️ This comment has been updated with latest results. |
pcanal
left a comment
pcanal
left a comment
There was a problem hiding this comment.
We should enhanced this PR by adding a new test that checks the current memory ownership explicitly (Otherwise we no longer test the current/main-stream usages)
That's a very good idea, and it's now implemented in the "master" PR for this feature. I'll cherry-pick the commit that will work already in ROOT 6 mode into this PR next. Edit: Done |
hageboeck
force-pushed
the
root7-ownership-9
branch
from
f39f216 to
d1c4b79
Compare
|
|
||
| TEST(TH1, RegistrationToTDirectory_ImplicitOwnershipOn) | ||
| { | ||
| TH1D histo1("histo1", "Test Histogram", 10, 0, 10); |
There was a problem hiding this comment.
Maybe (or maybe not) for the PR, we should also have similar for the other self Registering classes ....
There was a problem hiding this comment.
Duly noted. For TH1-derived classes, we do it in this PR. For other cases, I added a TODO to #18083
The test was relying on implicit ownership to transfer a histogram from python to C++, so now the histogram is appended explicitly.
The tutorial was running code that didn't directly reference the histogram, so it fails when implicit ownership is off.
hageboeck
force-pushed
the
root7-ownership-9
branch
from
d1c4b79 to
9a31bdc
Compare
hageboeck
force-pushed
the
root7-ownership-9
branch
from
9a31bdc to
5fe91b4
Compare
2 participants
In essence, add
histo.SetDirectory(xxx)to all places where a histogram should show up in a TDirectory independent of whether it has been registered automatically.