search

AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

GenericParameters should not return a default constructed / empty value if key is not present #576

Closed tmadlener closed 4 months ago

tmadlener commented 6 months ago

Currently the GenericParameters return an empty / default constructed value in case a key is not found:

https://github.com/AIDASoft/podio/blob/b94cc639b884356b8eb43b017586d86ed513558a/include/podio/GenericParameters.h#L216-L222

This currently makes it hard to impossible to check whether a value for a given key has been explicitly set to an empty / default value, or whether no value was set for a given key. Arguably for vectors and strings an empty value could indicate an unset value, but for int, float, double the "special value" of 0 might actually be a valid value for some parameters and it should be possible to distinguish that from an unset value.

The most straight forward solution in this case would be to make the return value a std::optional. The main drawback is that std::optional does not allow us to return a const & any longer so we will potentially introduce some more copies. Since set (and setValue) currently allow to reset values, this is also a way to invalidate these const &, so I think returning a copy instead of a const ref would by somewhat desirable from a thread safety point of view.

@wdconinc @nathanwbrei not sure how prevalent the use of these parameters are for the EIC. They are also used for Frame::putParameter and Frame::getParameter, so a change in behavior for the GenericParameters might also be visible when using Frames.

wdconinc commented 6 months ago

I just ran a few searches, and it doesn't seem like we use GenericParameters anywhere. The linked PR should be good to merge from our point of view. Thanks for checking.

tmadlener commented 6 months ago

Just to be sure, if #580 is merged this will also affect the Frame interface as that will also start to return optional for getParameter calls.

wdconinc commented 6 months ago

I confirmed that with #580, the only breakage in our stack is in DD4hep (1.28 and master) at https://github.com/AIDASoft/DD4hep/blob/f08e3359b2538cbae320e9fb9ff51e3eb0d780bb/DDG4/edm4hep/Geant4Output2EDM4hep.cpp#L451 (since no PR yet, I fixed it for now by adding .value_or(1) at the end, though that's not backwards compatible).

tmadlener commented 6 months ago

Thanks for the thorugh check. Let me clean up #580 a bit then and take care of making the change transparent in DD4hep.