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

Subject: Re: [boost] [Hana] Formal review for Hana
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2015-06-16 09:39:42

On Tue, Jun 16, 2015 at 8:04 AM, Louis Dionne <ldionne.2_at_[hidden]> wrote:

>
> Zach Laine <whatwasthataddress <at> gmail.com> writes:
>
> > [...]
> >
> > - 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.
>
> Actually, I thought we had agreed on front/back drop_front/drop_back :-).
> Anyway, this is tracked by this issue [2]. It's not a 100% trivial change
> because that will mean that `drop` is replaced by `drop_front`, which can
> also take an optional number of elements to drop. Same goes for
> `drop_back`,
> which will accept an optional number of elements to drop from the end of
> the
> sequence.
>

That sounds good too.

> > - I find boost::hana::make<boost::hana::Tuple>(...) to be clunkier than
> >   boost::hana::_tuple<>, in many places.
>
> You can use boost::hana::make_tuple(...).
>

Right.  It's just that sometimes I just want to name the type.

> > - 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>);
>
> That is the incorrect workaround. The proper, Hana-idiomatic way to write
> what you need is:
>
>     namespace detail {
>         auto make_matrix_type = [](auto rows) {
>             auto nrows = hana::size(rows);
>             static_assert(nrows >= 1u,
>             "matrix_t<> requires at least one row");
>
>             auto ncolumns = hana::size(rows[hana::size_t<0>]);
>             auto uniform = hana::all_of(rows, [=](auto row) {
>                 return hana::size(row) == ncolumns;
>             });
>             static_assert(uniform,
>             "matrix_t<> requires tuples of uniform length");
>
>             using tuple_type = decltype(hana::flatten(rows));
>
>             return hana::type<matrix_t<tuple_type, nrows, ncolumns>>;
>         };
>     }
>
>     template <typename ...Rows>
>     using matrix = typename decltype(
>         detail::make_matrix_type(hana::make_tuple(std::declval<Rows>()...))
>     )::type;
>

While that definitely works, it also definitely leaves me scratching my
head.  Why doesn't

static const std::size_t columns = hana::size(HeadRow{});

Work in the code above, especially since

auto ncolumns = hana::size(rows[hana::size_t<0>]);

does?  That seems at least a little obscure as an interface.  I also could
not get hana::size(hana::type<HeadRow>); to work, btw.

By the way, see my pull request at [4]. I'm now passing all the tests, and
> with a compilation time speedup!
>

Very nice!

> >   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;}
>
> You made your life harder than it needed to be. First, you should have
> written
>
>     `decltype(hana::type<T>)::type::size`
>
> instead of
>
>     `hana::_type<T>::_::type`
>
> This `_` nested type is an implementation detail. But then, you realize
> that
> this is actually equivalent to `T::size`. Indeed, `decltype(type<T>)::type`
> is just `T`. So basically, what you wanted is
>
>     template <typename T>
>     constexpr bool some_predicate (hana::_type<T> x)
>     { return T::size == some_constant;}
>
> which looks digestible junior coworkers :-).
>

Ha!  Fair enough.

> > 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)
>
> I don't understand; take_if() is not an algorithm provided by Hana.
>

Right!  I meant filter().  Sorry.

> 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.
>
> Oh, so you're trying to do an element-wise assignment?

Yes.

> Since Hana expects
> functions to be pure in general and assignment is a side effect, this
> cannot
> be achieved with a normal algorithm. However, Hana provides some algorithms
> that alow side-effects. One of them is for_each, and the other is `.times`
> (from IntegralConstant). I suggest you could write the following:
>
>     hana::size(foo).times.with_index([&](auto i) {
>         using T = std::remove_reference_t<decltype(bar[i])>;
>         bar[i] = static_cast<T>(foo[i]);
>     });
>

Ok.  You also had this in the PR you made for Units-BLAS:

hana::size(xs).times.with_index([&](auto i) {
    xs[i] = ys[i];
});

... which is even simpler.  However, either is a pretty awkward way to
express an operation that I think will be quite often desired.  copy(),
copy_if(), move(), and move_if() should be first-order operations.  I also
want the functional algorithms, but sometimes I simply cannot use them....
Now where's my 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.
>
> Cool!
>
>
> >   * 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.
>
> That is curious, because the Travis build is on linux and does not need
> that.
> Did you set the path to your libc++ installation on Linux? The README
> states
> that you should use
>
>     -DLIBCXX_ROOT=/path/to/libc++
>

Ah, cool.  I don't think I read the README.

> Thanks for the review, Zach. I also appreciate all the comments you
> provided
> privately; they were super useful. And you were right about the dots in the
> names; I should have changed them before the review. :-)
>

Glad to help!

Zach