Duplicate entries if FairVolumeList

[TobiasStockmanns]

Describe the bug When running a simulation macro with PandaRoot a number of errors is created, all of the type:

[ERROR] FairVolumeList element: PixelActiveo5 VolId : 6032 already defined 6030
[ERROR] FairVolumeList element: PixelActiveo5oPartAss VolId : 6032 already defined 6031
[ERROR] FairVolumeList element: PixelActiveo5 VolId : 6032 already defined 6030
[ERROR] FairVolumeList element: PixelActiveo5oPartAss VolId : 6032 already defined 6031

The error shows up in the recent dev branch of FairRoot and is not present in v18.8.2

To Reproduce Steps to reproduce the behavior:

1. FairSoft dev, FairRoot dev
2. PandaRoot dev
3. run /macro/master/sim_complete.C

[dennisklein]

This is a consequence of fixing #1495, which removed duplicate searches. Now, the only check happens inside addVolume - which currently decides to print an error (the one you see). IMHO, we should just remove this error log from addVolume as it now communicates back to the caller whether the volume was added or not (f8525850592d31cbe445ecaf87cd37e641714809).

[ChristianTackeGSI]

First: I agree with Dennis.

Note: I think, we added the error message there, because the new implementation effectively (using unique_ptr) deletes the passed in FairVolume. So this was some sort of "You gave me a FairVolume, well, I deleted it. Maybe that's a real problem?".

For me it is still not clear, whether this is "expected" behaviour? If so, IMHO the caller that expects this behaviour should at least (using comments) document that this is "okay".

All that said:

1. Is this error happening anywhere in the FairRoot "testsuite" (vulgo examples)?

   If yes: Good, then we have some "expected" happenings and should address them (by documentation).

   If not: See next point

2. (If not) What is PandaRoot performing differently? If PandaRoot's usage of FairRoot is fine, then we maybe should add another example that actually utilized this behaviour?

[dennisklein]

we added the error message

If you mean recently, no, we did not add it. It was already there for decades.

[ChristianTackeGSI]

we added the error message

If you mean recently, no, we did not add it. It was already there for decades.

Ohhh, okay.

Then: With the move to unique_ptr it became even more important?

[dennisklein]

No, why?

[ChristianTackeGSI]

Before 7f0f7f185cbef80298aeedb368631d638c1782d6 the passed volume was not destructed (and leaked). So any user of addVolume could still continue using the FairVolume.

With the destruction the error message became a little more important.

IMHO, if we remove the error message, we probably should remove the (deprecated) void addVolume(FairVolume* elem) as well. It makes it too easy to continue using an already destroyed object.

[dennisklein]

So any user of addVolume could still continue using the FairVolume.

No, they could not. Even before the semantics was to transfer ownership. The caller had no chance to know otherwise.

[dennisklein]

Or let me ask like this: give me a usage description for the void addVolume(FairVolume* elem) which is not broken.