- 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::makeboost::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.
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,
hana::tuple
m;
Each row is a tuple. But the storage needs to be a single tuple, so
matrix<> is actually a template alias for:
template
matrix_t { /* ... */ };
There's a metafunction that maps from the declaration syntax to the
correct
representational type:
template
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::makehana::Tuple(
HeadRow{},
TailRows{}...
)
));
using type = matrix_t;
};
The problem with the code above, is that it works with a HeadRow tuple
type
that is a literal type (e.g. a tuple). It does not work
with
a tuple containing non-literal types (e.g. a
tupleboost::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'
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(y)}...
{ }
The implication is that if I construct a tuple from rvalues, I get a
tuple. 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::repeathana::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,
[&](auto i) {
hana::at(bar, i) =
static_cast>(
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
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
