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.

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

Improve productivity with C++20 concepts? #810

Closed
antonysigma opened this issue Jul 20, 2023 · 3 comments
Closed

Improve productivity with C++20 concepts? #810

antonysigma opened this issue Jul 20, 2023 · 3 comments

Comments

@antonysigma
Copy link
Contributor

antonysigma commented Jul 20, 2023

I like the meta-programming feature built into the HighFive library. Thank you!

I am wondering if I can contribute C++20 code to make the compiler error more readable for average users?

For example, I want the compiler to report the missing struct Deflate::apply() function directly, instead of printing 100+ lines to of failed recursive template instantiations.

examples/direct_chunk_write.cpp:98:11: error: no matching member function for call to 'add'
    props.add(Deflate{});
    ~~~~~~^~~
subprojects/highfive/include/highfive/bits/../H5PropertyList.hpp:87:10: note: candidate template ignored: constraints not satisfied [with P = (anonymous namespace)::Jpegls]
    void add(const P& property);
         ^
subprojects/highfive/include/highfive/bits/../H5PropertyList.hpp:86:15: note: because '(anonymous namespace)::Deflate' does not satisfy 'PropertyInterface'
    template <PropertyInterface P>
              ^
subprojects/highfive/include/highfive/bits/../H5PropertyList.hpp:65:9: note: because 'p.apply(hid)' would be invalid: no member named 'apply' in '(anonymous namespace)::Jpegls'
    { p.apply(hid) };

Here's the example C++20 concepts code:

diff --git a/include/highfive/H5PropertyList.hpp b/include/highfive/H5PropertyList.hpp
index cc4bf90..c40a848 100644
--- a/include/highfive/H5PropertyList.hpp
+++ b/include/highfive/H5PropertyList.hpp
@@ -57,6 +57,18 @@ class PropertyListBase: public Object {
     }
 };
 
+#if __cplusplus >= 202002L
+template<typename P>
+concept PropertyInterface = requires(P p, const hid_t hid)
+{
+    // The struct/class P must contain a public member function that accepts
+    // hid_t as the argument.
+    { p.apply(hid) };
+};
+
+#else
+#define PropertyInterface typename
+#endif
 
 ///
 /// \brief HDF5 property Lists
@@ -75,7 +87,7 @@ class PropertyList: public PropertyListBase {
     /// A property is an object which is expected to have a method with the
     /// following signature void apply(hid_t hid) const
     ///
-    template <typename P>
+    template <PropertyInterface P>
     void add(const P& property);
 
     ///
diff --git a/include/highfive/bits/H5PropertyList_misc.hpp b/include/highfive/bits/H5PropertyList_misc.hpp
index 57fc26d..8ca021f 100644
--- a/include/highfive/bits/H5PropertyList_misc.hpp
+++ b/include/highfive/bits/H5PropertyList_misc.hpp
@@ -71,7 +71,7 @@ inline void PropertyList<T>::_initializeIfNeeded() {
 }
 
 template <PropertyType T>
-template <typename P>
+template <PropertyInterface P>
 inline void PropertyList<T>::add(const P& property) {
@alkino
Copy link
Member

alkino commented Jul 20, 2023

Of course we can include C++20 code BUT it has to be protected, because HighFive is still compliant C++11.

@1uc
Copy link
Collaborator

1uc commented Jul 20, 2023

Since the above nicely decays to less modern C++ in a manner that works well for both older compilers and buggy compilers. I can see how this makes users life easier since it's plausible they'd need to implement Properties for properties we don't support (yet). I think we can add something like this.

@antonysigma
Copy link
Contributor Author

antonysigma commented Jul 20, 2023

For sure. I will submit a draft PR for the PropertyInterface concept. To test the reaction from Highfive developers.

Update: @1uc @alkino . Here you are: #811

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants