Skip to content

Conversation

@fuhlig1
Copy link
Member

@fuhlig1 fuhlig1 commented Apr 20, 2020

Add a function to add a container to the factory that also checks if the
container already exist. If the container already exist it is not added
again and kFALSE is returned. Otherwise it is added and kTRUE is returned.
The "normal" way to add containers is to use the TList data member directly
which will not report any errors when adding the container. Only when the
destructor of the runtime database is called one will see an error message

Error in TList::Delete: A list is accessing an object (0x55d2e2220550)
already deleted (list name = TList)


Checklist:

@fuhlig1
Copy link
Member Author

fuhlig1 commented Apr 27, 2020

Could you please check and merge.

@dennisklein
Copy link
Member

dennisklein commented Apr 27, 2020

The need for this kind of checking add function is very worrisome. What does a return value kFALSE mean? It tells the caller that he is not the owner of the object (because the FairContFact instance is). But how can this be in the first place? The caller should already know this.

@fuhlig1
Copy link
Member Author

fuhlig1 commented Apr 27, 2020

The need for this kind of checking add function is very worrisome. What does a return value kFALSE mean? It tells the caller that he is not the owner of the object (because the FairContFact instance is). But how can this be in the first place? The caller should already know this.

This only tells the caller that the framework could not add the object to the internal TList because an object with the same name already exist in the list. This is completely independent from the question who is the owner.

The current way which is used all over the place in FairRoot and any derrived project is directly access the Internal TList (which is public) and call TList->Add directly. This works since years until you do a stupid copy and paste error. To use internal data members is a very bad idea but since the code is heavily used we can' change it without breaking all existing code.

I agree that passing the raw pointer is not the desired way. Since the function is not used anywhere yet I will try to implement a proper interface.

@dennisklein
Copy link
Member

dennisklein commented Apr 27, 2020

This only tells the caller that the framework could not add the object to the internal TList because an object with the same name already exist in the list.

True. Still, if it is possible that your calling code tries to add the same object (not by name but by reference) twice to the TList (doesn't matter whether it is through FairContFact or directly on the TList member), it means that the calling code seems to have lost track whether it is the owner of the added object.

root [0] TList* l = new TList; // non-owning container
root [1] TObject* obj = new TObject; // caller owns obj
root [2] l->Add(obj); // caller owns obj
root [3] l->Add(obj); // caller owns obj
root [4] delete l; // fine
root [5]
root [5] l = new TList;
root [6] l->SetOwner(kTRUE); // l now owning container
root [7] l->Add(obj); // caller does not own obj any more
root [8] l->Add(obj); // caller knows it should not call Add any more, because obj holds a non-owning pointer
root [9] delete l;
Error in <TList::Delete>: A list is accessing an object (0x5632af764490) already deleted (list name = TList)

I do not object this PR, especially if you intend to have a TList that actually represents a unique set of TObjects by name. From your issue description I got the impression you wanted to fix a double add of the same object (by reference), which would indicate a problem in the calling code.

@dennisklein dennisklein added this to the v18.4 milestone Apr 27, 2020
@dennisklein dennisklein force-pushed the improve_faircontfact branch from 2b21e88 to 1b47a32 Compare April 29, 2020 08:51
Copy link
Member

@dennisklein dennisklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the comment to the new interface.

@dennisklein dennisklein force-pushed the improve_faircontfact branch from 1b47a32 to aee909e Compare April 29, 2020 08:58
Add a function to add a container to the factory that also checks if the
container already exist. If the container already exist it is not added
again and kFALSE is returned. Otherwise it is added and kTRUE is returned.
The "normal" way to add containers is to use the TList data member directly
which will not report any errors when adding the container. Only when the
destructor of the runtime database is called one will see an error message

Error in <TList::Delete>: A list is accessing an object (0x55d2e2220550)
already deleted (list name = TList)
@dennisklein dennisklein force-pushed the improve_faircontfact branch from aee909e to 57ff172 Compare April 29, 2020 13:46
@MohammadAlTurany MohammadAlTurany merged commit c2e38e3 into FairRootGroup:dev Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants