[Tree] Delete virtual copy and copy assignment methods in TTree #19227

guitargeek · 2025-06-28T11:18:22Z

The TTree class already deletes the copy constructor and the copy assignment, but users might still be tempted to use the virtual copy constructor or copy assignment provided by TObject::Clone() and TObject::Copy(). But these implementations also make no sense to the TTree.

To avoid user confusion, this commit suggests to override these methods in TTree such that they thow an exception and tell you what to use instead.

Prevents reports like this one in the future:

https://its.cern.ch/jira/browse/ROOT-9111

github-actions · 2025-06-28T15:51:08Z

Test Results

22 files 22 suites 3d 21h 59m 6s ⏱️
3 689 tests 3 688 ✅ 0 💤 1 ❌
79 225 runs 79 223 ✅ 1 💤 1 ❌

For more details on these failures, see this check.

Results for commit 099cfa9.

♻️ This comment has been updated with latest results.

ferdymercury · 2025-07-02T14:47:46Z

Tangentially related: #9252

pcanal · 2025-07-09T19:21:15Z

Note that TTree::Clone is used (intentionally) internally to implement CloneTTree.
I.e. the CI failures (at least most of them) are related and require further adjustment to the code.

pcanal · 2025-07-10T03:09:08Z

roofit/roofitcore/src/RooTreeDataStore.cxx

+  std::unique_ptr<TTree> tClone{static_cast<TTree*>(const_cast<TTree*>(t)->CloneTree())};
+  tClone->SetDirectory(nullptr);


Consider setting gDirectory to nullptr (via the TContext) to ensure that the TTree is really in memory.

Suggested change

std::unique_ptr<TTree> tClone{static_cast<TTree*>(const_cast<TTree*>(t)->CloneTree())};

tClone->SetDirectory(nullptr);

{

TDirectory::TContext ctx{nullptr};

std::unique_ptr<TTree> tClone{static_cast<TTree*>(const_cast<TTree*>(t)->CloneTree())};

}

but humm ... the previous code and the new code are doing different things (not clear what the 'real' intent is or should have been).

old: Copy just the TTree data members themselves and attach the copy to the same file:
net effect: There is 2 concurrent ways to look at the same data in the same file. Each copy of the TTree can have their own branch address and their own TTreeCache. They don't affect each other (no change in address and no changing where the cursor is).

new: Copy the tree and all its data .. but in the same file (if it is writeable) well actually in whatever file is current This is very slow if the TTree is big. The 2 trees do not affect each others but they also have duplicated (potential large) data.

proposed: Load all the data in memory after possibly a slow copy. This may or may not fit in memory. (To speeds the copy up, one could consider attaching to a TMemFile).

So really the essential question before deciding which way to go is to understand what was the intent/point of the cloning of the meta data and what are the performance and memory usage limitation that this routine needs to fit in.

Suggested change

std::unique_ptr<TTree> tClone{static_cast<TTree*>(const_cast<TTree*>(t)->CloneTree())};

tClone->SetDirectory(nullptr);

std::unique_ptr<TTree> tClone{static_cast<TTree*>(t->GetDirectory()->CloneObject(t));

is another option ...

pcanal · 2025-07-10T03:11:28Z

tree/tree/src/TTree.cxx


+TObject *TTree::Clone(const char *) const
+{
+   throw std::runtime_error("TObject::Clone() is not supported by TTree! Use TTree::CloneTree().");


Minor: I think this is the first exception thrown in this file :)

I agree with Philippe and I think it shouldn't be implemented this way, rather via

Error("Clone", "Not implemented, use XXXXXXX instead.");

roofit/roofitcore/src/RooDerivative.cxx

vepadulano · 2025-07-10T12:52:44Z

roofit/roofitcore/src/RooTreeDataStore.cxx

  // of the original, it tears it down at destruction time!
  auto deleter = [](TTree* tree){tree->SetDirectory(nullptr); delete tree;};
-  std::unique_ptr<TTree, decltype(deleter)> tClone(static_cast<TTree*>(t->Clone()), deleter);
+  std::unique_ptr<TTree, decltype(deleter)> tClone{static_cast<TTree*>(t->GetDirectory()->CloneObject(t)), deleter};


What is the fundamental difference between tree->Clone and tree->GetDirectory()->CloneObject(tree) that makes the latter ok but the first not?

vepadulano · 2025-07-10T12:54:49Z

tree/tree/src/TTree.cxx


+TObject *TTree::Clone(const char *) const
+{
+   throw std::runtime_error("TObject::Clone() is not supported by TTree! Use TTree::CloneTree().");


I agree with Philippe and I think it shouldn't be implemented this way, rather via

Error("Clone", "Not implemented, use XXXXXXX instead.");

tree/tree/src/TTree.cxx

vepadulano

Thank you for this work! I do not disagree with the goal of this PR, but before approval I wanted to clarify a point

From what I can see, this PR disallows calling TTree::Clone and TTree::Copy, but in practice we are still cloning TTree ourselves in our own code in two ways:

via TDirectory::CloneObject
by calling the base class method TNamed::Clone

I just want to make sure that before we go ahead we are aware of what we are doing and think wether there could be better alternatives. Do we need to clone the TTree in those occasions? If so, why don't we use CloneTree? At least I believe we should add some comments inplace.

vepadulano · 2025-10-20T19:28:23Z

bindings/pyroot/pythonizations/test/memory.py

-            "TH1D": ("h", "h", 10, 0, 10),
            "TH1K": ("h", "h", 10, 0, 10),
            "TProfile": ("h", "h", 10, 0, 10),
            "TH2C": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2S": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2I": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2L": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2F": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2D": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2Poly": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH2PolyBin": tuple(),
            "TProfile2D": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TProfile2PolyBin": tuple(),
            "TProfile2Poly": ("h", "h", 10, 0, 10, 10, 0, 10),
            "TH3C": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TH3S": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TH3I": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TH3L": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TH3F": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TH3D": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TProfile3D": ("h", "h", 10, 0, 10, 10, 0, 10, 10, 0, 10),
            "TGraph2D": (100,),
            "TEntryList": ("name", "title"),
            "TEventList": ("name", "title"),
-            "TTree": ("name", "title"),
-            "TNtuple": ("name", "title", "x:y:z"),


Why are these three entries removed from the test cases?

The `TTree` class already deletes the copy constructor and the copy assignment, but users might still be tempted to use the virtual copy constructor or copy assignment provided by `TObject::Clone()` and `TObject::Copy()`. But these implementations also make no sense to the TTree. To avoid user confusion, this commit suggests to override these methods in TTree such that they thow an exception and tell you what to use instead. Prevents reports this one in the future: * https://its.cern.ch/jira/browse/ROOT-9111

guitargeek self-assigned this Jun 28, 2025

guitargeek requested a review from pcanal as a code owner June 28, 2025 11:18

guitargeek added improvement in:TTree labels Jun 28, 2025

guitargeek force-pushed the ROOT-9111 branch from 72ce715 to d05d6de Compare June 28, 2025 12:01

guitargeek requested a review from lmoneta as a code owner June 28, 2025 13:11

guitargeek force-pushed the ROOT-9111 branch from 2601d6b to 3affc90 Compare July 9, 2025 22:36

guitargeek requested a review from couet as a code owner July 9, 2025 22:36

pcanal reviewed Jul 10, 2025

View reviewed changes

roofit/roofitcore/src/RooDerivative.cxx Show resolved Hide resolved

guitargeek force-pushed the ROOT-9111 branch from 3affc90 to 333f2e5 Compare July 10, 2025 11:28

guitargeek requested review from dpiparo and vepadulano as code owners July 10, 2025 11:28

guitargeek force-pushed the ROOT-9111 branch 2 times, most recently from 9cf5898 to fb16d02 Compare July 10, 2025 12:22

vepadulano reviewed Jul 10, 2025

View reviewed changes

guitargeek force-pushed the ROOT-9111 branch 2 times, most recently from 7566084 to 3291911 Compare July 15, 2025 11:09

vepadulano requested changes Oct 20, 2025

View reviewed changes

guitargeek force-pushed the ROOT-9111 branch from 3291911 to 099cfa9 Compare October 22, 2025 19:26

		std::unique_ptr<TTree> tClone{static_cast<TTree>(const_cast<TTree>(t)->CloneTree())};
		tClone->SetDirectory(nullptr);

Uh oh!

[Tree] Delete virtual copy and copy assignment methods in TTree #19227

Are you sure you want to change the base?

[Tree] Delete virtual copy and copy assignment methods in TTree #19227

Uh oh!

Conversation

guitargeek commented Jun 28, 2025 • edited by dpiparo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ferdymercury commented Jul 2, 2025

Uh oh!

pcanal commented Jul 9, 2025

Uh oh!

pcanal Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

pcanal Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

pcanal Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

vepadulano Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vepadulano Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

vepadulano Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

vepadulano Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guitargeek commented Jun 28, 2025 •

edited by dpiparo

Loading

github-actions bot commented Jun 28, 2025 •

edited

Loading