From 099cfa951a51f8c9e865aebeb132c85cad407ad8 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sat, 28 Jun 2025 13:13:33 +0200 Subject: [PATCH] [Tree] Delete virtual copy and copy assignment methods in TTree 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 --- bindings/pyroot/pythonizations/test/memory.py | 5 +---- io/io/src/TFileMerger.cxx | 2 +- roofit/roofitcore/src/RooTreeDataStore.cxx | 2 +- tree/tree/inc/TTree.h | 3 +++ tree/tree/src/TTree.cxx | 13 ++++++++++++- 5 files changed, 18 insertions(+), 7 deletions(-) diff --git a/bindings/pyroot/pythonizations/test/memory.py b/bindings/pyroot/pythonizations/test/memory.py index 71c43302718d8..357d8710a4e60 100644 --- a/bindings/pyroot/pythonizations/test/memory.py +++ b/bindings/pyroot/pythonizations/test/memory.py @@ -49,7 +49,7 @@ def test_ttree_clone_in_file_context(self): ttree = ROOT.TTree("tree", "tree") - with ROOT.TFile(filename, "RECREATE") as infile: + with ROOT.TFile(filename, "RECREATE") as _: ttree_clone = ttree.CloneTree() os.remove(filename) @@ -88,7 +88,6 @@ def test_objects_ownership_with_subdir(self): "TH1I": ("h", "h", 10, 0, 10), "TH1L": ("h", "h", 10, 0, 10), "TH1F": ("h", "h", 10, 0, 10), - "TH1D": ("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), @@ -111,8 +110,6 @@ def test_objects_ownership_with_subdir(self): "TGraph2D": (100,), "TEntryList": ("name", "title"), "TEventList": ("name", "title"), - "TTree": ("name", "title"), - "TNtuple": ("name", "title", "x:y:z"), } for klass, args in objs.items(): with self.subTest(klass=klass): diff --git a/io/io/src/TFileMerger.cxx b/io/io/src/TFileMerger.cxx index b9889b72a96a3..4db125d6fde14 100644 --- a/io/io/src/TFileMerger.cxx +++ b/io/io/src/TFileMerger.cxx @@ -529,7 +529,7 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type, && !cl->InheritsFrom( TDirectory::Class() )) { R__ASSERT(cl->IsTObject()); TDirectory::TContext ctxt(current_sourcedir); - obj = obj->Clone(); + obj = gDirectory->CloneObject(obj); ownobj = kTRUE; } } else if (key) { diff --git a/roofit/roofitcore/src/RooTreeDataStore.cxx b/roofit/roofitcore/src/RooTreeDataStore.cxx index 79e4e6ca5f31b..c1f0ea0a41ece 100644 --- a/roofit/roofitcore/src/RooTreeDataStore.cxx +++ b/roofit/roofitcore/src/RooTreeDataStore.cxx @@ -369,7 +369,7 @@ void RooTreeDataStore::loadValues(const TTree *t, const RooFormulaVar* select, c // We need a custom deleter, because if we don't deregister the Tree from the directory // of the original, it tears it down at destruction time! auto deleter = [](TTree* tree){tree->SetDirectory(nullptr); delete tree;}; - std::unique_ptr tClone(static_cast(t->Clone()), deleter); + std::unique_ptr tClone{static_cast(gDirectory->CloneObject(t)), deleter}; tClone->SetDirectory(t->GetDirectory()); // Clone list of variables diff --git a/tree/tree/inc/TTree.h b/tree/tree/inc/TTree.h index 7609529df00fe..788253ce221d7 100644 --- a/tree/tree/inc/TTree.h +++ b/tree/tree/inc/TTree.h @@ -354,6 +354,9 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker TTree(const TTree& tt) = delete; TTree& operator=(const TTree& tt) = delete; + TObject *Clone(const char *newname="") const override; + void Copy(TObject &object) const override; + virtual Int_t AddBranchToCache(const char *bname, bool subbranches = false); virtual Int_t AddBranchToCache(TBranch *branch, bool subbranches = false); virtual Int_t DropBranchFromCache(const char *bname, bool subbranches = false); diff --git a/tree/tree/src/TTree.cxx b/tree/tree/src/TTree.cxx index 3f100ace22eb5..e0cbf75c0fe12 100644 --- a/tree/tree/src/TTree.cxx +++ b/tree/tree/src/TTree.cxx @@ -1059,6 +1059,17 @@ TTree::~TTree() } } +TObject *TTree::Clone(const char *) const +{ + Error("Clone", "Not implemented, use TTree::CloneTree instead."); + return nullptr; +} + +void TTree::Copy(TObject &) const +{ + Error("Copy", "Not implemented, use TTree::CopyTree or TTree::CopyEntries instead."); +} + //////////////////////////////////////////////////////////////////////////////// /// Returns the transient buffer currently used by this TTree for reading/writing baskets. @@ -3204,7 +3215,7 @@ TTree* TTree::CloneTree(Long64_t nentries /* = -1 */, Option_t* option /* = "" * // Note: For a chain, the returned clone will be // a clone of the chain's first tree. - TTree* newtree = (TTree*) thistree->Clone(); + TTree* newtree = (TTree*) thistree->TNamed::Clone(); if (!newtree) { return nullptr; }