BlueBrain / HighFive

HighFive - Header-only C++ HDF5 interface
https://bluebrain.github.io/HighFive/
Boost Software License 1.0
677 stars 160 forks source link

Improve productivity with C++20 concepts? #810

Closed antonysigma closed 1 year ago

antonysigma commented 1 year ago

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 commented 1 year ago

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

1uc commented 1 year ago

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 commented 1 year ago

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: https://github.com/BlueBrain/HighFive/pull/811