-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
Fix Attribute copy/move constructors #1545
Conversation
!std::is_same_v< | ||
std::remove_cv_t<std::remove_reference_t<T>>, | ||
Attribute>, | ||
T> &&val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would specializing be simpler/cleaner than enable if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be better, yes. We would need to do this for every openPMD datatype then, though.
Probably best at that point to introduce something like the datatype macros that ADIOS2 has? openPMD_FOREACH_DATATYPE
or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that needed? The specialization could be something like
template <typename T>
Attribute(Attribute<T> const &) = default;
template <typename T>
Attribute(Attribute<T> &&) = default;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template specialization doesn't work in this case: With template
, we cannot use default
, without template
, it's not a specialization, but an overload that is never chosen.
Using a specialization doesn't fix the underlying problem. In the current form, the constructor template <typename T> Attribute(T &&val)
accepts anything, but is supposed to work only for all openPMD datatypes. That's a wrong interface.
Since the Intel Compiler doesn't understand this use of enable_if
, I'd suggest using a macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best would be a C++20 concept here though :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I propose is no template specialization but an overload for the constructors. Did you try if that works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you did another approach with macros now ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I propose is no template specialization but an overload for the constructors. Did you try if that works?
That's exactly what this PR does. Just, it doesn't write all ~30 constructors explicitly, but uses a macro for that instead.
Copy and move constructors of the
Attribute
class were accidentally custom-defined by having a constructor of the form:Hence, be a bit more careful: