- Whether you believe the library should be accepted into Boost Yes. * Conditions for acceptance I have no explicit conditions without satisfaction of which Hana should not proceed, but I have some recommendations for changes below. - Your name Zach Laine - Your knowledge of the problem domain. I'm knowledgeable about metaprogramming and value+type programming, a la Fusion. - How much effort did you put into your evaluation of the review? I partially converted an existing TMP-heavy library for doing linear algebra using heterogeneously-typed Boot.Units values in matrices. I spent about 8 hours on that. I also have been to all Louis' talks and have looked thoroughly (though not used) previous versions of the library. You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design Overall, very good. In my partial conversion mentioned above, I was able to cut out lots of boilerplate code, that essentially just iterated over tuple elements, with Hana algorithms. I was able to use hana::tuple as a replacement for std::tuple relatively painlessly. The code looked smaller and just *better* using Hana than it did when I hand-rolled it. That is significant, IMO. I have been saying since I first converted this same library over to C++14 code that I have no interest in MPL or any other such thing, since metaprogramming involving values is largely trivial in C++14, and all metaprogramming is much easier. See Peter Dimov's recent blog post for a more thorough analysis of this than I could ever write. With all of that, I still found I was able to reduce the code in my library by using Hana. There are some things that I consider worth changing, however: - The use of '.' in names instead of '_' (Louis has already said that he will change this). - The inclusion of multiple functions that do the same thing. Synonyms such as size()/length() and fold()/fold.left() sow confusion. As a maintainer of code that uses Hana, it means I have to intermittently stop and refer back to the docs. Hana is already quite large, and there's a lot there to try to understand! - I would prefer that the names of things that are opposites be obviously so in their naming. For instance, filter()/remove_if() are a opposites -- they do the same thing, just with the predicate inverted. Could these be called remove_if()/remove_if_not()? I (and I have heard this from others as well) have worked in a place that had multiple functions called filter() in the codebase, at least one of which meant filter-as-in-filter-out, and at least one of which meant filter-as-in-keep. filter() may be a more elegant spelling than the admittedly-clunky remove_if_not(), but the latter will not require me to refer to the docs when reviewing code. - I'd like to see first() / last() / after_first() / before_last() instead of head() / last() / tail() / init() (this was suggested by Louis offline), as it reinforces the relationships in the names. - I find boost::hana::make<boost::hana::Tuple>(...) to be clunkier than boost::hana::_tuple<>, in many places. Since I want to write this in some cases, it would be nice if it was called boost::hana::tuple<> instead, so it doesn't look like I'm using some implementation-detail type. * Implementation Very good; mind-bending in some of the implementation details. However, there are some choices that Louis made that I think should be changed: - The inclusion of standard headers was forgone in an effort to optimize compile times further (Hana builds quickly, but including e.g. <type_traits> increases the include time of hana.hpp by factors). This is probably not significant in most users' use of Hana; most users will include Hana once and then instantiate, instantiate, instantiate. The header-inclusion time is just noise in most cases, and is quite fast enough in others. However, rolling one's own std::foo, where std::foo is likely to evolve over time, is asking for subtle differences to creep in, in what users expect std::foo to do vs. what hana::detail::std::foo does. What happens if I use std::foo in some Hana-using code that uses hana::detail::std::foo internally? Probably, subtle problems. Moreover, this practice introduces an unnecessary maintenance burden on Hana to follow these (albeit slowly) moving targets. - I ran in to a significant problem in one case that suggests a general problem. In the linear algebra library I partially reimplemented, one declares matrices like this (here, I use the post-conversion Hana tuples): matrix< hana::tuple<a, b>, hana::tuple<c, d>
m;
Each row is a tuple. But the storage needs to be a single tuple, so matrix<> is actually a template alias for: template <typename Tuple, std::size_t Rows, std::size_t Columns> matrix_t { /* ... */ }; There's a metafunction that maps from the declaration syntax to the correct representational type: template <typename HeadRow, typename ...TailRows> struct matrix_type { static const std::size_t rows = sizeof...(TailRows) + 1; static const std::size_t columns = hana::size(HeadRow{}); /* ... */ using tuple = decltype(hana::flatten( hana::make<hana::Tuple>( HeadRow{}, TailRows{}... ) )); using type = matrix_t<tuple, rows, columns>; }; The problem with the code above, is that it works with a HeadRow tuple type that is a literal type (e.g. a tuple<float, double>). It does not work with a tuple containing non-literal types (e.g. a tuple<boost::units::quantity<boost::units::si::time>, /* ... */>). This is a problem for me. Why should the literalness of the types contained in the tuple determine whether I can know its size at compile time? I'd like to be able to write this as a workaround: static const std::size_t columns = hana::size(hana::type<HeadRow>); I'd also like Hana to know that when it sees a type<> object, that is should handle it specially and do the right thing. There may be other compile-time-value-returning algorithms that would benefit from a similar treatment. I was unable to sketch in a solution to this, because hana::_type is implemented in such a way as to prevent deduction on its nested type (the nested type is actually hana::_type::_::type, not hana::_type::type -- surely to prevent problems elsewhere in the implementation). As an aside, note that I was forced to name this nested '_' type elsewhere in a predicate. This is not something I want my junior coworkers to have to read: template <typename T> constexpr bool some_predicate (hana::_type<T> x) { return hana::_type<T>::_::type::size == some_constant;} Anyway, after trying several different ways to overcome this, I punted on using hana::size() at all, and resorted to: static const std::size_t columns = HeadRow::size; ... which precludes the use of std::tuple or my_arbitrary_tuple_type with my matrix type in future. - I got this error: error: object of type 'boost::hana::_tuple<float, float, float, float>' cannot be assigned because its copy assignment operator is implicitly deleted This seems to result from the definitions of hana::_tuple and hana::detail::closure. Specifically, the use of the defaulted special member functions did not work, given that closure's ctor is declared like this: constexpr closure_impl(Ys&& ...y) : Xs{static_cast<Ys&&>(y)}... { } The implication is that if I construct a tuple from rvalues, I get a tuple<T&&, U&&, ...>. The fact that the special members are defaulted is not helpful, since they are still implicitly deleted if they cannot be generated (e.g. the copy ctor, when the members are rvalue refs). I locally removed all the defaulted members from _tuple and closure, added to closure a defined default ctor, and changed its existing ctor to this: constexpr closure_impl(Ys&& ...y) : Xs{::std::forward<Ys>(y)}... { } ... and I stopped having problems. My change might not have been entirely necessary (I think that the static_cast<> might be ok to leave in there), but something needs to change, and I think tests need to be added to cover copies, assignment, moves, etc., of tuples with different types of values and r/lvalues. * Documentation Again, this is overall very good. Some suggestions: - Make a cheatsheet for the data types, just like the ones for the algorithms. - Do the same thing for the functions that are currently left out (Is this because they are not algorithms per se?). E.g. I used hana::repeat<hana::Tuple>(), but it took some finding. * Tests - The tests seem quite extensive. * Usefulness Extremely useful. I find Hana to be a faster, *much* easier-to-use MPL, and an easier-to-use Fusion, if only because it shares the same interface as the MPL-equivalent part. There are other things in there I haven't used quite yet that are promising as well. A lot of the stuff in Hana feels like a solution in search of a problem -- but that may just be my ignorance of Haskell-style programming. I look forward to one day understanding what I'd use hana::duplicate() for, and using it for that. :) That being said, I really, really, want to do this: tuple_1 foo; tuple_2 bar; boost::hana::copy_if(foo, bar, [](auto && x) { return my_predicate(x); }); Currently, I cannot. IIUC, I must do: tuple_1 foo; auto bar = boost::hana::take_if(foo, [](auto && x) { return my_predicate(x); }); Which does O(N^2) copies, where N = boost::hana::size(bar), as it is pure-functional. Unless the optimizer is brilliant (and it typically is not), I cannot use code like this in a hot section of code. Also, IIUC take_if() cannot accommodate the case that decltype(foo) != decltype(bar) What I ended up doing was this: hana::for_each( hana::range_c<std::size_t, 0, hana::size(foo)>, [&](auto i) { hana::at(bar, i) = static_cast<std::remove_reference_t<decltype(hana::at(bar, i))>>( hana::at(foo, i) ); } ); ... which is a little unsatisfying. I also want move_if(). And a pony. - Did you attempt to use the library? If so: * Which compiler(s) Clang 3.6.1, on both Mac and Linux. My code did actually compile (though did not link due to the already-known bug) on GCC 5.1. * What was the experience? Any problems? I had to add "-lc++abi" to the CMake build to fix the link on Linux, but otherwise no problems. Zach On Wed, Jun 10, 2015 at 4:19 AM, Glen Fernandes <glen.fernandes@gmail.com> wrote:
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