FairRoot
FairRoot copied to clipboard
FairRootGroup
Ownership semantics of `FairRootManager::GetObject`
- [ ] Analyze usage in FairRunAnaProof, FairRunOnline of GetObject
- #1369
- #1396 https://github.com/FairRootGroup/FairRoot/blob/12f519f8054d50d0d0edc345878f93a8b76a9699/fairroot/online/steer/FairRunOnline.cxx#L221
- [ ] Cleanup FairRootManager and GetObject
- #1353
- #1354
- #1402
- [ ] Fix some (related) memory handling
- #1358
Started by this discussion: https://github.com/FairRootGroup/FairRoot/pull/1340#discussion_r1099872737
FairRootManager::GetObject returns a TObject*. It is not obvious, whether this pointer is an owning or non-owning pointer.
Some code assumes it to be owning and consequently deletes the object at the end. Others don't.
The current implementation of this member function highly suggests that it returns non-owning pointers, because it usually returns copies of pointers (for example stored in fObj2–see little cleanup in #1343). @rbx came to this conclusion in the above mentioned discussion and I fully agree.
First the intended semantics should be clearly documented.
If it should return a non-owning pointer, then all callers should be analyzed to no longer delete the received pointer and FairRootManager should clean them up at the end (since it is owning the pointer).
If it should return an owning pointer, we should have an API that returns a unique_ptr (possibly already casted to the correct type).
It should return a non-owning pointer. The reason for this is that there may be several tasks requiring the pointer.
An interesting question from this morning:
Should callers of GetObject be able to modify the object they get this way? If not, then GetObject maybe should return a const TObject*?
Ideally it should be a const TObject &, or? Then it is clear that the manager maintains the ownership.
I guess GetObject could return a nullptr, if the thing isn't found?
What we need is something like a unique_ptr, just with no ownership semantics. In theory we could use a shared_ptr. But I don't know, if this really is the way to go, or whether it's just the biggest hammer.
An interesting question from this morning:
Should callers of GetObject be able to modify the object they get this way? If not, then GetObject maybe should return a
const TObject*?
That is a good question. On one hand there may be cases, where the data modification is good, but generally I would say the data should not be modified.
However. Simple example of the time based reconstruction and FairTSBufferFunctional.
The fInputArray is obtained via FRM::GetObject():
https://github.com/FairRootGroup/FairRoot/blob/master/base/steer/FairTSBufferFunctional.cxx#L42
and here it is absorbed into fOutputArray:
https://github.com/FairRootGroup/FairRoot/blob/master/base/steer/FairTSBufferFunctional.cxx#L54
But looking at the description of the AbsorbObjects function(s):
https://root.cern.ch/doc/master/classTClonesArray.html#a457c60a37d9260ce77b446c154125496
it takes the ownership of the fInputArray and rearranges it completely.
It would not be that simple with const TObject*, or?