Add an static_assert for the GenericParameters and remove the EnableIf #676
Conversation
There was a problem hiding this comment.
I think the overload with std::vector<std::string> also needs some changes, because without SFINAE, putParameter("somekey", {"an", "initializer_list", "of", "string", "literals"}), will hit the one with std::initializer_list<T>, which will trigger a compilation error once it gets to the underlying GenericParameters.
|
Also similar changes should be put in place for |
|
CI failure seems to be due to an issue in EDM4hep. |
|
CI for edm4hep workflows should start to work again with #677 |
| @@ -264,7 +264,7 @@ def put_parameter(self, key, value, as_type=None): | |||
| cpp_types = _get_cpp_types(as_type) | |||
| if len(cpp_types) == 0: | |||
| raise ValueError(f"Cannot put a parameter of type {as_type} into a Frame") | |||
| self._frame.putParameter[cpp_types[0]](key, value) | |||
| self._frame.putParameter[cpp_types[0][0]](key, value) | |||
There was a problem hiding this comment.
Why is this change necessary? Was this a pre-existing bug?
There was a problem hiding this comment.
cpp_types will be a list of tuples so the first [0] gets the first tuple. Before since there were two template arguments (one of them being the EnableIf) it was possible to pass this tuple, now it is not, and the second [0] gets the actual C++ type. So not exactly a bug. While having a look at this I also tried to simplify this part a bit but in the end I left it as it is with only a couple of lines in a more pythonic way.
|
I think I got all of them now.
This one actually has to remain, because otherwise the |
include/podio/Frame.h
Outdated
| inline void putParameter(const std::string& key, T value) { | ||
| static_assert(podio::isSupportedGenericDataType<T>, "The type is not supported by the GenericParameters"); |
There was a problem hiding this comment.
| static_assert(podio::isSupportedGenericDataType<T>, "The type is not supported by the GenericParameters"); | |
| static_assert(podio::isSupportedGenericDataType<T>, "T is not a supported parameter type"); |
I think GenericParameters is too much of an implementation detail here, especially if users get this message from their algorithms, they will not know what it is and where to look. I would change this message at least for the cases in Frame. Not sure if we should also put the supported types in, because this message is likely to become outdated if we have to add more types.
There was a problem hiding this comment.
For the same reason T doesn't tell you much where to look or what to do. But showing the type isn't completely trivial 🤷
There was a problem hiding this comment.
We need to have documentation where we state the supported datatypes (not sure if we already have). I think we could also drop the T from the message. In the end the compiler error will contain the type they tried to use in some form.
There was a problem hiding this comment.
The docs mention int, float, strong and vectors of those https://github.com/AIDASoft/podio/blob/master/doc/frame.md#usage-for-parameters
There was a problem hiding this comment.
Ah right, I knew it was somewhere :D. But it's missing a double 🙈
…o std::string when trying to put a parameter that can be converted to std::string, therefore it will fail with the static_assert when compiling.
…tring (#680) when trying to put a parameter that can be converted to std::string, therefore it will fail with the static_assert when compiling. Co-authored-by: jmcarcell <jmcarcell@users.noreply.github.com>
This was discussed in key4hep/k4FWCore#223 (comment).
BEGINRELEASENOTES
static_assertin its place to make errors easier to read and debug.ENDRELEASENOTES