AOD: fixe memory leak + allow overwriting metadata

[shahor02]

No description provided.

[ktf]

Suggested change

	LOGP(warnING, "The table \"{}\" is not valid and will not be saved!", tableName);
	LOGP(warning, "The table \"{}\" is not valid and will not be saved!", tableName);

[sawenzel]

What is the problem here? ta2tr does not seem to be a pointer. Is this deleting an object returned by process() which is not owned by ta2tr? This looks a bit awkward.

[shahor02]

TableToTree returns the pointer to the tree after the writing. It has only a default constructor, so although it has a pointer data member, it is not deleted. The alternative would be to define it as unique_ptr, but then its potential consumer should be aware of this.

[jgrosseo]

Hi Ruben,
Why we need the case of overwriting the metadata?

[shahor02]

@jgrosseo because currently, it produces FATAL when metadata is there

[jgrosseo]

@jgrosseo because currently, it produces FATAL when metadata is there

Yes, that is not good of course. I would simply change that to WARNING then. The overwrite does not harm, but does not help either...

[shahor02]

@jgrosseo do you want simply skip creating the metada if it is already there?

[shahor02]

@sawenzel @ktf I've changed the raw mTree pointer in the TableToTree to shared one and removed the deletion of the returned pointer.

[ktf]

@shahor02 i would actually check if we cannot simply avoid returning the tree.

[jgrosseo]

@jgrosseo do you want simply skip creating the metada if it is already there?

Yes, exactly. It will be identical (unless we have a quite bad setup...)

[shahor02]

@ktf check with whom? At the moment the returned pointer is never used, but I understood the returning was done on purpose in case somebody will need this pointer.

[shahor02]

@jgrosseo ok, I've changed the code to preserve the existing one

[ktf]

@shahor02 what i meant is that the pointer might have been used, so before changing it to anything else, we should have checked. If it's not used, I would be in favour of simply deleting the object inside the function and not return anything.

[shahor02]

Hi @ktf, currently the pointer is not used in any real code, but is used in the test:

AliceO2/Framework/Core/test/test_TreeToTable.cxx

Lines 150 to 154 in b82fa60

	auto br = (TBranch*)t2->GetBranch("ok");
	BOOST_REQUIRE_EQUAL(t2->GetEntries(), ndp);
	BOOST_REQUIRE_EQUAL(br->GetEntries(), ndp);
	br = (TBranch*)t2->GetBranch("tests");
	BOOST_REQUIRE_EQUAL(br->GetEntries(), ndp);

Let me know if you want to preserve it.
In any case, to have this fix in the nightly, I would merge this PR once the CIs are green, then, if decided, can avoid returning a pointer and implement a destructor.

[ktf]

yeah... I guess the correct thing is to return a unique ptr, then. We can merge this and think about it later.

[shahor02]

yeah... I guess the correct thing is to return a unique ptr, then. We can merge this and think about it later.

For the record: at the moment it returns shared ptr, which is destroyed since never assigned.

[ktf]

Yes, sure, that would work as well. I guess the unique_ptr is maybe better / safer, but we can always change it later.