[hana] Lorenzo's Formal review for Hana
Hello Glen, Louis, and all, Please find below my review of the proposed Boost.Hana library.
- Whether you believe the library should be accepted into Boost
Yes, Boost.Hana should be accepted into Boost. I decided to make my acceptance vote unconditional. However, I do have some concerns and I hope these can be addressed if Hana is in fact accepted into Boost. I marked these concerns as "[CONCERN]" below.
- Your name
Lorenzo Caminiti.
- Your knowledge of the problem domain.
I have been using C++ Template Meta-Programming (TMP) for 10+ years now. I routinely maintain ~23,000 lines of production code that extensively uses Boost.MPL, Boost.Fusion, and a TMP-based serialization mechanism (among a number of other Boost libraries) -- compilation times are a *huge* issue with this code!
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
The library API seems reasonable, consistent, and powerful.
* [CONCERN] Assertions should be renamed from ..._CHECK to ..._ASSERT
(to be consistent with assert, static_assert, BOOST_STATIC_ASSERT,
BOOST_MPL_ASSERT, etc.).
* Is there a ..._MSG version for the assertion macros?
(These should probably just variadic macros where a second optional
parameter represents the error message to display in case the asserted
condition if false.)
* The following is very nice!
auto has_toString = is_valid([](auto&& obj) ->
decltype(obj.toString()) { });
I've been wanting to do the above at function scope for a long time
(so I can program it within the function where I actually need it).
Now with C++14 generic lambdas we finally can :)
Also this is very nice!
template <typename T>
std::string optionalToString(T const& obj) {
return if_(has_toString(obj),
[](auto& x) { return x.toString(); },
[](auto& x) { return "toString not defined"; }
)(obj);
}
Somewhat similar to my static_if (renamed call_if, after Vicente's
suggestion) proposal using C++14 generic lambdas:
http://boost.2283326.n4.nabble.com/static-if-Is-there-interest-in-a-static-i...
BTW, how would you program the following possible implementation of
std::advance in Hana?
template<typename Iter>
struct is_random_access_iterator : std::is_same<
typename std::iterator_traits<Iter>::iterator_category,
std::random_access_iterator_tag
> {};
template<typename Iter>
struct is_bidirectional_iterator : std::is_same<
typename std::iterator_traits<Iter>::iterator_category,
std::bidirectional_iterator_tag
> {};
template<typename Iter>
struct is_input_iterator : std::is_same<
typename std::iterator_traits<Iter>::iterator_category,
std::input_iterator_tag
> {};
template
* Implementation
Some code can be optimized, but the author already mentioned he will look into that this summer. I did not see any specific issue otherwise.
* Documentation
* [CONCERN] The documentation will be much (much!) more readable is the "hana" namespace was not stripped away. Please write hana::..., mpl::... (if not the full boost::hana::..., boost::mpl::...) in the docs. * The docs say: "Notice how we cast the result of x.member to void? This is to make sure that our detection also works for types that can't be returned from functions, like array types." auto has_member = is_valid([](auto&& x) -> decltype((void)x.member) { }); But obviously, the following (without the cast) would work too, correct? auto has_member = is_valid([](auto&& x) -> decltype(x.member, void()) { }); * I would expect the following example to use operator|| instead of or_: auto ts = filter(types, [](auto t) { return or_(is_pointer(t), is_reference(t)); }); * Performance graphs would be more readable if they always used the same color for the same entity. For example, sometimes mpl::vector is blue, other times it is orange... * Why the "Runtime performance of reverse on normal container" for fusion::vector stops at "Number of elements" = 50?
* Tests
* Usefulness
* If not for anything else, I will use this library for its SFINAE facilities. Second, I will use this library in production code for its improved compilation times (assuming these can be confirmed on MSVC). Finally, I am curious to use this library in general because it seems it will improve my meta-programming, also making easier to read. * [CONCERN] The author should be committed to support the library on GCC and MSVC once that becomes feasible, even if that requires a good deal of workarounds. For example, Boost.Preprocessor is full of MSVC workaround (because MSVC preprocessor is still not standard compliant... after 30+ years of existence of the C preprocessor). Maybe MSVC will never be 100% C++14 standard compliant... I don't know. But I do know that if this library is not ported to MSVC at some future point, I won't be able to use it in my production code. I do hope that adoption into Boost will push vendors to support C++14 constructs used by this library TMP techniques. I also hope the author is committed to do whatever it takes to make the library work on MSVC. * [CONCERN] What compiler was used to measure performance? CLang? I hope the promised performance gains (especially compile-time reductions) will be there for GCC, and especially later also for MSVC. If not, that will significantly limit my motivation on using this library in production code. TMP compilation times are largely dependant on specific compiler implementations of C++ template machinery which might vary significantly from vendor to vendor. Therefore, at the moment I only know that the library is expected to compile faster on Clang, but unfortunately that does not tell me anything about expected compilation time improvements on GCC, and even less on MSVC...
- Did you attempt to use the library? If so: * Which compiler(s)?
Clang.
* What was the experience? Any problems?
Overall, pretty good experience. No specific issue to report a part from what already noted.
- How much effort did you put into your evaluation of the review?
A total of about 2 days between reading docs, playing with some
examples, and looking at the code a bit.
My thanks to Luis for his outstanding work with this library!
--Lorenzo
On Wed, Jun 10, 2015 at 2:19 AM, Glen Fernandes
Dear Boost community,
The formal review of Louis Dionne's Hana library begins today,10th June and ends on 24th June.
Hana is a header-only library for C++ metaprogramming that provides facilities for computations on both types and values. It provides a superset of the functionality provided by Boost.MPL and Boost.Fusion but with more expressiveness, faster compilation times, and faster (or equal) run times.
To dive right in to examples, please see the Quick start section of the library's documentation: http://ldionne.com/hana/index.html#tutorial-quickstart
Hana makes use of C++14 language features and thus requires a C++14 conforming compiler. It is recommended you evaluate it with clang 3.5 or higher.
Hana's source code is available on Github: https://github.com/ldionne/hana
Full documentation is also viewable on Github: http://ldionne.github.io/hana
To read the documentation offline: git clone http://github.com/ldionne/hana --branch=gh-pages doc/gh-pages
For a gentle introduction to Hana, please see: 1. C++Now 2015: http://ldionne.github.io/hana-cppnow-2015 (slides) 2. C++Con 2014: https://youtu.be/L2SktfaJPuU (video) http://ldionne.github.io/hana-cppcon-2014 (slides)
We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance - Your name - Your knowledge of the problem domain.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design * Implementation * Documentation * Tests * Usefulness - Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems? - How much effort did you put into your evaluation of the review?
We await your feedback!
Best, Glen
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Lorenzo Caminiti
[...]
* [CONCERN] Assertions should be renamed from ..._CHECK to ..._ASSERT (to be consistent with assert, static_assert, BOOST_STATIC_ASSERT, BOOST_MPL_ASSERT, etc.).
There are actually `BOOST_HANA_..._ASSERT` macros. However, the _ASSERT versions are disabled in release mode, while the _CHECK versions are not. Since I want the conditions to be checked at all times in the examples and unit tests, I use the _CHECK version.
* Is there a ..._MSG version for the assertion macros? (These should probably just variadic macros where a second optional parameter represents the error message to display in case the asserted condition if false.)
No, there are no such macros. Actually, I'm not sure whether those assertion macros should be part of the library's interface at all. Indeed, there is no perfect way to implement them and I think it would be better if they were not used by users. If so, I should move them to detail/ and document that they just give semantic information about the nature of the condition for the documentation.
[...]
BTW, how would you program the following possible implementation of std::advance in Hana?
[...]
I might write something like this:
template
* This macro:
struct Person { BOOST_HANA_DEFINE_STRUCT(Person, (std::string, name), (int, age) ); };
Will it work even if the data member types contain commas? [...]
Yes, it will also work.
* [CONCERN] The docs say: "Hana's design assumes that most of the time, we want to access all or almost all the elements in a sequence anyway, and hence performance is not a big argument in favor of laziness." Especially given that other well-established libraries like Fusion addressing the same problem domain have gone in favor of laziness instead... Is this a good assumption? If so, why?
This assumption is based mainly on two things: 1. From my personal experience, I don't remember ever relying on Fusion's lazy semantics. Admittedly, this is a poor rationale. However... 2. For validation, I asked my GSoC mentor, Joel Falcou, whether the lazy semantics were an important part of Fusion that should be preserved. The answer was basically that it made the implementation of Fusion easier, but that it was not a crucial aspect. Also, we couldn't take advantage of C++11/14 features as much with lazy semantics (in particular parameter packs) to make the compile-times better. So it would have been a C++11/14 reimplementation of Fusion, without much design changes. I think time will tell whether this was a good idea, but in all cases it would be possible to add lazy views to Hana in the future if we realize this was a mistake.
* [CONCERN] The docs say: "By default, Hana assumes functions to be pure. A pure function is a function that has no side-effects at all." Will this assumption limit what users will be able to do with Hana?
To be pedantic, no, because of Turing completeness. In practice, I also do not think it will be a problem. However, this is really a "FP vs the world" thing. Some things will be better expressed functionally, and some things will be better expressed with mutation, but that's an old debate. Hana has gone on the road of purity, but I introduced loopholes in the system to make trivial stuff achievable without conceptual overhead. This is the case of `for_each`, for example, which takes a function that can have side effects. It also guarantees the order in which the function is called, as you would expect. Without this "loophole", we would need to use Monads to achieve side effects. I thought this was overkill and would hinder the adoption of the library in a C++ context. In summary; you do almost everything with pure functions, and when it becomes painful you have loopholes that allow mutation and side effects.
Do MPL and Fusion relay on a similar assumption? Yes/no, why...
For MPL, this is not relevant because it is type-level computations only.
Regarding Fusion, I don't think there are any mentions of whether the
functions are allowed to have side effects. For example, is the following
well-defined?
fusion::vector
[...]
* [CONCERN] Rename Map, Range, etc. to map, range, etc. (all lower case). In Boost and STL capital letters are only used for template parameters, and maybe concepts. (If it is important to distinguish that those are tags (I don't think so) then name them map_tag, range_tag, etc. but still no capital letters.)
This was already suggested a couple of times. Added an issue [2] to consider it.
* [CONCERN] is_empty, length, and few other names should be changed to empty, size, etc. to match STL naming conventions (so I don't have to remember different names for the same thing).
You can use `size` in place of `length` if you prefer; an alias is provided for consistency with the STL. Regarding `empty` vs `is_empty`: `empty` is already used to denote an empty container (`empty<Tuple>()`, `empty<Optional>()`, etc..) Furthermore, regardless of the choices made in the STL, I find `is_empty` to be arguably more descriptive than `empty`. I try to have some consistency with the STL, but I think diverging is OK when I judge a STL name to be really bad.
[...]
* Documentation
* [CONCERN] The documentation will be much (much!) more readable is the "hana" namespace was not stripped away. Please write hana::..., mpl::... (if not the full boost::hana::..., boost::mpl::...) in the docs.
This will be changed. See this issue [3].
* The docs say: "Notice how we cast the result of x.member to void? This is to make sure that our detection also works for types that can't be returned from functions, like array types."
auto has_member = is_valid([](auto&& x) -> decltype((void)x.member) { });
But obviously, the following (without the cast) would work too, correct?
auto has_member = is_valid([](auto&& x) -> decltype(x.member, void()) { });
Yes, I would expect the second to work as well.
* I would expect the following example to use operator|| instead of or_:
auto ts = filter(types, [](auto t) { return or_(is_pointer(t), is_reference(t)); });
Currently, the traits return std::integral_constants, not Hana's integral constants. However, this is part of this issue [4].
* Performance graphs would be more readable if they always used the same color for the same entity. For example, sometimes mpl::vector is blue, other times it is orange...
You're right. See this issue [5].
* Why the "Runtime performance of reverse on normal container" for fusion::vector stops at "Number of elements" = 50?
Because you can't create a fusion::vector with more than 50 elements.
[...]
* [CONCERN] The author should be committed to support the library on GCC and MSVC once that becomes feasible,
Yes.
even if that requires a good deal of workarounds.
No. I'm coding against a standard, not a specific compiler. If I introduce workarounds in Hana, the whole purpose of the library is lost. Performance, ease of use, and most importantly hackability will be lost. Quite honestly, I'd rather have my library never get into Boost than bastardize it so it works on MSVC. If it can be made to work on MSVC without bastardizing it, however, I'm all for it.
[...]
Maybe MSVC will never be 100% C++14 standard compliant... I don't know. But I do know that if this library is not ported to MSVC at some future point, I won't be able to use it in my production code.
Anyone is welcome to try and create a MSVC compliant fork of the library. If it works out well, I can guarantee I'll merge everything in the main branch.
I do hope that adoption into Boost will push vendors to support C++14 constructs used by this library TMP techniques.
I hope and think it will (if the library is accepted).
I also hope the author is committed to do whatever it takes to make the library work on MSVC.
* [CONCERN] What compiler was used to measure performance? CLang? I hope the promised performance gains (especially compile-time reductions) will be there for GCC, and especially later also for MSVC. If not, that will significantly limit my motivation on using this library in production code.
So far, performance gains were measured on Clang. However, older benchmarks I made for MPL11 were also on GCC, and they showed similar gains. So I think the modern techniques used in Hana will provide a speedup on both GCC and Clang. Once GCC is fully supported, I will also run benchmarks on GCC and make them available in the documentation.
TMP compilation times are largely dependant on specific compiler implementations of C++ template machinery which might vary significantly from vendor to vendor. Therefore, at the moment I only know that the library is expected to compile faster on Clang, but unfortunately that does not tell me anything about expected compilation time improvements on GCC, and even less on MSVC...
We'll be able to have different algorithm implementations on different compilers if there's a need for it. So in all cases, I expect Hana should be able to beat anything you'd write by hand (naively).
- Did you attempt to use the library? If so: * Which compiler(s)?
Clang.
* What was the experience? Any problems?
Overall, pretty good experience. No specific issue to report a part from what already noted.
- How much effort did you put into your evaluation of the review?
A total of about 2 days between reading docs, playing with some examples, and looking at the code a bit.
My thanks to Luis for his outstanding work with this library!
--Lorenzo
Thanks a lot for the review, Lorenzo. Regards, Louis [1]: http://goo.gl/MPiZx0 [2]: https://github.com/ldionne/hana/issues/157 [3]: https://github.com/ldionne/hana/issues/126 [4]: https://github.com/ldionne/hana/issues/92 [5]: https://github.com/ldionne/hana/issues/151
participants (2)
-
Lorenzo Caminiti -
Louis Dionne