Reading TFiles and histograms into shared_ptr, and TH1 constructors

[ychiu]

Dear experts,

I am learning how to use smart pointers. I want to read a series of ROOT files and load histograms from each file – while managing the object ownership and memory allocation myself or with smart pointers.
Here is a snippet of my code, with questions in-line:

1. According to [1], ROOT does not manage TFiles, so I do not need to set m_File->AddDirectory(0). Correct? Here, I am constructing a TFile object on the heap with the default c’tor, with the file path forwarded to the constructor. m_File is a std::shared_ptr.

if (boost::filesystem::exists(inputFilePath)) {
    m_File = std::make_shared<TFile>(inputFilePath.string().c_str()));
}

2. The following snippet works. But I think it can be simplified by simply passing h directly into the copy constructor of hclone (which does a Copy()). Is this preferred?

for (auto hn : histogramNames) {
    for (auto p : processMap) {
        std::shared_ptr<TFile> pfile = p.second.File(); // File() does return-by-value of m_File
        if (pfile->GetListOfKeys()->Contains(hn.c_str())) {
            TH1 *h;
            pfile->GetObject(hn.c_str(), h);
            std::shared_ptr<TH1D> hclone = std::make_shared<TH1D>(*(static_cast<TH1D*>(h->Clone())));
            h->AddDirectory(0);
            hclone->AddDirectory(0);
            rank.emplace(h->Integral(), h); // emplace into multimap of <float, std::shared_ptr<TH1>>
    }
}

            TH1 *h;
            pfile->GetObject(hn.c_str(), h);
            std::shared_ptr<TH1D> hclone = std::make_shared<TH1D>(static_cast<TH1D>(h));
            h->AddDirectory(0);
            hclone->AddDirectory(0);

3. Since I am managing the objects with smart pointers, I need to set AddDirectory(0) on both h and hclone. Since Clone() returns a TObject *, I suspect that there is a memory allocation here that is not cleaned up if I choose to remove it from gDirectory. Is this correct?

(Histograms in different ROOT files share the same name and I can’t have TObjects with the same name in the same gDirectory, so either I need to Clone("...") with a unique string or set AddDirectory(0). This must be done regardless of using smart pointers or not…)

4. If the histogram read in from the file, h, is a TH1I, it will turn into a TH1D because of the cast. This is not really efficient. How can I make this snippet of code more flexible? This does not work (TH1::TH1(const TH1&)’ is private within this context):

std::shared_ptr<TH1> hclone = std::make_shared<TH1>(static_cast<TH1>(h));

5. How does this work with THStacks and canvases? Is it safe toAdd(TH1 *h) to a THStack, where every h inside the TList of the stack has the same name and whose ownership is not managed by ROOT?

Thanks,
Justin

[1] https://root.cern.ch/root/htmldoc/guides/users-guide/ObjectOwnership.html

[behrenhoff]

Hi there,

first of all: are you sure you need a std::shared_ptr? By default, use a std::unique_ptr. You probably have one data structure to manage ownership. Shared pointers can be useful at times, but in most cases what you really want is clear, simple ownership. Then unique pointers are the way to go. ROOT even contains the include header ROOT/RMakeUnique.hxx to provide std::make_unique if you are not yet on C++14.

Comments to your code:

for (auto hn : histogramNames) {
This should be
for (const auto &hn : histogramNames) {
to avoid copying and to disable the possibility to change the histogram name.

In general, make all your loop variables const&. Leaving out const means your will change the object. Leaving out the & means making a copy. You can do that for simple types like int.

The whole block that follows (it seems the closing } is missing) after the std::shared_ptr<TFile> pfile = p.second.File(); does NOTHING to the ownership of the file - it is just using the file to read an object. It doesn’t care who owns it. Therefore don’t copy the smart pointer, use a simple pointer instead. Your process map File() method should probably return the simple pointer as well. I don’t know your code and if you are sharing ownerhip of the files. But usually it is best to have just one object owning the files.

Next thing:
if (pfile->GetListOfKeys()->Contains(hn.c_str())) {
You don’t need that, remove it!
Instead, check the result of pfile->GetObject(hn.c_str(), h);!
That is, do if (h) { object_found; } else { not_found; } afterwards.
In your code, you continue even if you didn’t get the object (because of incompatible types, for example).

As for the next line:
std::shared_ptr<TH1D> hclone = std::make_shared<TH1D>(*(static_cast<TH1D*>(h->Clone())));
First of all: avoid repeating the type that often:
-> auto hclone = std::make_shared<TH1D>(*(static_cast<TH1D*>(h->Clone())));
Then what are you doing here? Cloning + casting + dereferencing + calling the copy constructor + leaking an object? You also make your code dependent on TH1D.
So: cast to TH1* instead of the specific type. That why we the Clone mehtod exists in the first place! Then clone already IS a pointer to the copy, no need deref + forward to the copy constuctor.
auto hclone = std::shared_ptr<TH1>(static_cast<TH1*>(h->Clone())); or
std::shared_ptr<TH1> hclone(static_cast<TH1*>(h->Clone()));.
Again, you should be using a unique_ptr instead:
std::unique_ptr<TH1> hclone(static_cast<TH1*>(h->Clone())); or the auto = unique_ptr... variant.
Rank should probably be using unique_ptrs as well.

Your second alternative is plain WRONG:
std::shared_ptr<TH1D> hclone = std::make_shared<TH1D>(static_cast<TH1D>(h));
h is a TH1*, so you can’t cast it into TH1D. This doesn’t even compile. What is that supposed to do?

Question 3: true.

Question 4: already answered above. You are missing the call to Clone and the * in your cast. You cannot directly call the base class copy constructor on polymorphic types (base class copy would only copy the base class members!) and expect the complete “real” type to be copied. “Clone” is necessary when you have polymorphism.

As for general advice on ownership and smart pointers in C++, see also
F.7: For general use, take T* or T& arguments rather than smart pointers
F.26: Use a unique_ptr to transfer ownership where a pointer is needed
F.27: Use a shared_ptr to share ownership (see the Note and Alternative sections)

There is also a section on cloning:
C.67: A base class should suppress copying, and provide a virtual clone instead if “copying” is desired

[pcanal]

Also ‘AddDirectory’ is a static function that affect all future creation of histogram (asking them to default to not be attached to a directory). You usually only need to call this function once.

In your code, you probably meant to use SetDirectory where you used AddDirectory.

Cheers,
Philippe.