Possible leak with dynamic_cast and TKey::ReadObj?

Newbie
1

I am looking at the TKey::ReadObj() docs for ROOT 6.28. The “can create a new object […] This new object […]” lets me assume, that ReadObj returns an owning pointer (the implementation code also looks like this)?

A bit down under “Example2” it recommends to use dynamic_cast to cast to MyClass. If the TKey actually contains a MyClass, then everything is fine. But if it contains something else (that is not “castable” to MyClass), then I think, this code will produce a leak? (Obviously a static_cast is even worse.)

If this does not leak, I would like to learn how this works internally.

If this could leak, I would suggest to at least add a small warning to the documentation?

And I would like to learn, what is the recommended way to not leak (assuming MyClass is derived from TObject)?

I can think of two variants:

  1. Handling it manually:

    TObject *tobj = key->ReadObj();
auto mything = dynamic_cast<MyClass*>(tobj);
if (!mything) {
    delete tobj;
}

  2. Using TKey::ReadObject<T>():

    auto mything = key->ReadObject<MyClass>();

    But the documentation looks like this is not intended for classes deriving from TObject? (although the code looks like it handles that?)

Can you please recommend on how to handle things?

2

You are right :frowning:

Handling it manually:

Indeed you code works.

But the documentation looks like this is not intended for classes deriving from TObject?

The new interface can be used for TObject.

1 Like
3

So would you recommend using TKey::ReadObject<T>()?

This seems quite compact and nice? (I would have liked a std::unique_ptr to convey that it is an owning pointer).

4

So would you recommend using TKey::ReadObject<T>()?

Yes.

(I would have liked a std::unique_ptr to convey that it is an owning pointer).

Not quite always. For TTree, TH* and a couple of other it is actual shared between TFile and the user.

1 Like
5

Hmmm. But you’re still allowed to delete those via the pointer returned from ReadObj / ReadObject<T>, right? Otherwise the manual checking variant (1) would be wrong (let’s assume the key points to a TTree, but I was looking for MyClass).

Assuming you’re always allowed to delete what is returned from those methods, a std::unique_ptr wouldn’t be too wrong? It would just delete in cases, where TFile would also do it later on?

The problem would arise, if the TFile gets destructed before the TTree, because then we would have a double delete?

6

But you’re still allowed to delete those via the pointer returned from ReadObj / ReadObject<T>, right?

Yes.

The problem would arise, if the TFile gets destructed before the TTree, because then we would have a double delete?

This is correct.

Assuming you’re always allowed to delete what is returned from those methods, a std::unique_ptr wouldn’t be too wrong?

It would give the wrong ‘indication/semantic’ for TTree and TH1 as for those there is a shared ownership (with the issue you pointed out).

We attempted to upgrade the return type of ReadObject but we failed to find a way that was backward compatible.

7

Yeah. As the return type is not part of the signature, it does not take part in overload resolution. The only way around this is to introduce a new name. Something like Get<T>() (like in TFile::Get). I don’t know. I don’t really have a good suggestion.

That said, the other question would be: What would be the correct return type? As you pointed out, std::unique_ptr is not 100% correct (it’s correct as long, as the TFile is alive). A std::shared_ptr would match things more correctly (for TH1 & Co, TFile would also have a std::shared_ptr, for all the others, the returned shared_ptr would be the single owner). But implementing the old ReadObj / ReadObject will be a small headache.