fix memory leak in VectorPDF

[veprbl]

Resolves: eic/EICrecon#1456

cc #36

[veprbl]

Guarding memory deallocation behind a macro would expose users of root io enabled builds to memory leak. TObject instances are not owned in ROOT, so the code in this PR is valid in both cases.

…

On May 20, 2024, at 10:31 AM, Christopher Dilks ***@***.***> wrote:  @c-dilks commented on this pull request. In include/SinglePDF.h: > + ~VectorPDF() { + for(auto pdf : m_Members) { + delete pdf; + } + }; Since this class inherits from TObject, does streaming instances of VectorPDF to ROOT files still work? If not, or you don't want to test, perhaps use DISABLE_ROOT_IO: ⬇️ Suggested change - ~VectorPDF() { - for(auto pdf : m_Members) { - delete pdf; - } - }; + ~VectorPDF() { +#ifdef DISABLE_ROOT_IO + for(auto pdf : m_Members) { + delete pdf; + } +#endif + }; — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[c-dilks]

Guarding memory deallocation behind a macro would expose users of root io enabled builds to memory leak. TObject instances are not owned in ROOT, so the code in this PR is valid in both cases.

No double free in this case?

auto f = new TFile("f.root");
auto v = new VectorPDF();
// ... things that fill `m_Members` ...
v->Write("v");
f->Close();

[veprbl]

Guarding memory deallocation behind a macro would expose users of root io enabled builds to memory leak. TObject instances are not owned in ROOT, so the code in this PR is valid in both cases.

No double free in this case?

auto f = new TFile("f.root");
auto v = new VectorPDF();
// ... things that fill `m_Members` ...
v->Write("v");
f->Close();

This wouldn't even work, it's not TNamed. But yes, there is no reason to get double free from that.

[c-dilks]

This wouldn't even work, it's not TNamed.

It works on ROOT 6.28; TObjects may be written.

But yes, there is no reason to get double free from that.

Ship it! ROOT I/O for this repo is disabled in the spack builds anyway.

[c-dilks]

v1.0.8 tagged since eic/EICrecon#1456 is closed.

[wdconinc]

Can someone confirm that this will get ported to IRT2 as well, or included automatically?

[c-dilks]

Hopefully; see

• feat!: IRT 2.0 #34
• feat: update pfrich branch with respect to main #35

[veprbl]

This wouldn't even work, it's not TNamed.

It works on ROOT 6.28; TObjects may be written.

Sure, "obviously", you're right, TKey's can be created for TObject, by calling TObject::Write.

[wdconinc]

@veprbl Why did we not catch the memory leak in irt with lsan? Is one of the root suppressions hiding it? Can we narrow it?

[veprbl]

One needs to compile with lsan to detect the leaks.

[wdconinc]

Ah right, of course. Duh.