Re: [boost] [Hana] Formal review for Hana

Subject: Re: [boost] [Hana] Formal review for Hana
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2015-06-15 11:17:01

- 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_at_[hidden]>
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
>