Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an static_assert for the GenericParameters and remove the EnableIf #676

Merged
merged 7 commits into from
Sep 18, 2024
Merged

Add an static_assert for the GenericParameters and remove the EnableIf #676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f411e01
Add an static_assert for the GenericParameters and remove the EnableIf
jmcarcell Sep 17, 2024
7a22860
Add a static_assert for getParameters
jmcarcell Sep 17, 2024
7593bd6
Remove other EnableIfs
jmcarcell Sep 18, 2024
103b1ac
Simplify related python code
jmcarcell Sep 18, 2024
8c20100
Add a comment about the indexing
jmcarcell Sep 18, 2024
54c682d
Change the error message
jmcarcell Sep 18, 2024
e899f11
Change error message
jmcarcell Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions include/podio/Frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,9 @@ class Frame {
/// is supported by GenericParameters
/// @param key The name under which this parameter should be stored
/// @param value The value of the parameter. A copy will be put into the Frame
template <typename T, typename = podio::EnableIfValidGenericDataType<T>>
template <typename T>
inline void putParameter(const std::string& key, T value) {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
m_self->parameters().set(key, std::move(value));
}

Expand All @@ -246,8 +247,8 @@ class Frame {
///
/// @param key The name under which this parameter should be stored
/// @param value The value of the parameter. A copy will be put into the Frame
inline void putParameter(const std::string& key, std::string value) {
putParameter<std::string>(key, std::move(value));
inline void putParameter(const std::string& key, const char* value) {
putParameter<std::string>(key, value);
}

/// Add a vector of strings value the parameters of the Frame.
Expand Down Expand Up @@ -282,8 +283,9 @@ class Frame {
/// @param key The key under which the value is stored
///
/// @returns An optional holding the value if it is present
template <typename T, typename = podio::EnableIfValidGenericDataType<T>>
template <typename T>
inline auto getParameter(const std::string& key) const {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
return m_self->parameters().get<T>(key);
}

Expand All @@ -302,8 +304,9 @@ class Frame {
/// @tparam T The desired parameter type
///
/// @returns A vector of keys for this parameter type
template <typename T, typename = podio::EnableIfValidGenericDataType<T>>
template <typename T>
inline std::vector<std::string> getParameterKeys() const {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
return m_self->parameters().getKeys<T>();
}

Expand Down
24 changes: 14 additions & 10 deletions include/podio/GenericParameters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,16 @@ class GenericParameters {

~GenericParameters() = default;

template <typename T, typename = EnableIfValidGenericDataType<T>>
template <typename T>
std::optional<T> get(const std::string& key) const;

/// Store (a copy of) the passed value under the given key
template <typename T, typename = EnableIfValidGenericDataType<T>>
template <typename T>
void set(const std::string& key, T value);

/// Overload for catching const char* setting for string values
void set(const std::string& key, std::string value) {
set<std::string>(key, std::move(value));
void set(const std::string& key, const char* value) {
set<std::string>(key, std::string(value));
}

/// Overload for catching initializer list setting of string vector values
Expand All @@ -113,11 +113,11 @@ class GenericParameters {
size_t getN(const std::string& key) const;

/// Get all available keys for a given type
template <typename T, typename = EnableIfValidGenericDataType<T>>
template <typename T>
std::vector<std::string> getKeys() const;

/// Get all the available values for a given type
template <typename T, typename = EnableIfValidGenericDataType<T>>
template <typename T>
std::tuple<std::vector<std::string>, std::vector<std::vector<T>>> getKeysAndValues() const;

/// erase all elements
Expand Down Expand Up @@ -202,8 +202,9 @@ class GenericParameters {
mutable MutexPtr m_doubleMtx{nullptr}; ///< The mutex guarding the double map
};

template <typename T, typename>
template <typename T>
std::optional<T> GenericParameters::get(const std::string& key) const {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
const auto& map = getMap<T>();
auto& mtx = getMutex<T>();
std::lock_guard lock{mtx};
Expand All @@ -221,8 +222,9 @@ std::optional<T> GenericParameters::get(const std::string& key) const {
}
}

template <typename T, typename>
template <typename T>
void GenericParameters::set(const std::string& key, T value) {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
auto& map = getMap<T>();
auto& mtx = getMutex<T>();

Expand All @@ -248,8 +250,9 @@ size_t GenericParameters::getN(const std::string& key) const {
return 0;
}

template <typename T, typename>
template <typename T>
std::vector<std::string> GenericParameters::getKeys() const {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
std::vector<std::string> keys;
const auto& map = getMap<T>();
keys.reserve(map.size());
Expand All @@ -262,8 +265,9 @@ std::vector<std::string> GenericParameters::getKeys() const {
return keys;
}

template <typename T, typename>
template <typename T>
std::tuple<std::vector<std::string>, std::vector<std::vector<T>>> GenericParameters::getKeysAndValues() const {
static_assert(podio::isSupportedGenericDataType<T>, "Unsupported parameter type");
std::vector<std::vector<T>> values;
std::vector<std::string> keys;
auto& mtx = getMutex<T>();
Expand Down
8 changes: 5 additions & 3 deletions python/podio/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

def _get_cpp_types(type_str):
"""Get all possible c++ types from the passed py_type string."""
types = list(filter(lambda t: type_str in t, SUPPORTED_PARAMETER_TYPES))
types = [t for t in SUPPORTED_PARAMETER_TYPES if type_str in t]
if not types:
raise ValueError(f"{type_str} cannot be mapped to a valid parameter type")

Expand All @@ -43,7 +43,7 @@ def _get_cpp_vector_types(type_str):
"""Get the possible std::vector<cpp_type> from the passed py_type string."""
# Gather a list of all types that match the type_str (c++ or python)
types = _get_cpp_types(type_str)
return [f"std::vector<{t}>" for t in map(lambda x: x[0], types)]
return [f"std::vector<{t[0]}>" for t in types]


def _is_collection_base(thing):
Expand Down Expand Up @@ -264,7 +264,9 @@ 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)
# The first [0] gets the tuple, the second [0] gets the actual C++ type
# from SUPPORTED_PARAMETER_TYPES
self._frame.putParameter[cpp_types[0][0]](key, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary? Was this a pre-existing bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


# If we have a single integer, a std::string overload kicks in with higher
# priority than the template for some reason. So we explicitly select the
Expand Down