-
Notifications
You must be signed in to change notification settings - Fork 160
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
Constrain the Property object at compile-time #811
Conversation
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
=======================================
Coverage 84.58% 84.58%
=======================================
Files 68 68
Lines 4781 4781
=======================================
Hits 4044 4044
Misses 737 737
|
Hi HighFive project owners @alkino . I noticed that the PR passes all CI validations except |
Thank you for the nice PR. Yes, please feel free to fix the formatting; I think the version of
I've not had time today to compile it and see how it works myself. Though I'm looking forward to having a reason to look at concepts. |
include/highfive/H5PropertyList.hpp
Outdated
/// \sa Instructions to document C++20 concepts with Doxygen: https://github.com/doxygen/doxygen/issues/2732#issuecomment-509629967 | ||
/// | ||
/// \cond | ||
#if __cplusplus >= 202002L |
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.
@antonysigma I would like there to be a way of forcing the concepts to be off. I'm worried a user will find a compiler that claims to be 202002L
but doesn't support concepts; or has a compiler bug. If this happens they would be unable to use HighFive, because a nice, but non-essential feature is broken.
If you like please add a CMake variable HIGHFIVE_HAS_CONCEPTS
which defaults to On
. Then add code to instruct the compiler to add -DHIGHFIVE_HAS_CONCEPTS={0,1}
has appropriate. Then, the if-condition above would be
#if HIGHFIVE_HAS_CONCEPTS && __cplusplus >= 202002L
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.
Done. I am not familiar to CMake >12.0 though. Feel free to revise the changes where it make sense.
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.
Thanks! I slightly changed it and tested if it works. One might need -fconcepts-diagnostics-depth=2
to get meaningful compiler output.
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.
Great! Let me know if you need anything else. I will standby.
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'd like to prepare a PR which modifies CI to also compile HighFive as C++20 code. Otherwise, the concepts aren't tested during CI. Then we can rebase this PR onto the other one; and everything is tested nicely again.
Thank you for this nice PR.
24cee49
to
cd10241
Compare
The property struct/class requires a public member function signature `apply(hid_t) const`. Describe the constrain in the C++20 concepts syntax. The primarily goal is to make compiler errors more readable for average users. Average users are those who do not understand SFINAE.
cd10241
to
4c4ede1
Compare
The commit sets the variable for the target `libdeps` (and transitively `HighFive`). Furthermore, it ensures that the value of `HIGHFIVE_HAS_CONCEPTS` is either `0` or `1`. Finally, it removes the check for C++ standard, because we already check if the compiler should be able to handle C++20 features via `__cplusplus` in the code.
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.
Thank you, nicely done!
Description
The property struct/class requires a public member function signature
apply(hid_t) const
. Describe the constrain in the C++20 concepts syntax. The primarily goal is to make compiler errors more readable for average users. Average users are those who do not understand SFINAE.Resolves feature #810 .
How to test this?
It is a compile-time type checking. If the production code or test code compiles, the code is valid.
Test System