Re: [boost] [hana] Lorenzo's Formal review for Hana

Subject: Re: [boost] [hana] Lorenzo's Formal review for Hana
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2015-06-24 11:41:11

Lorenzo Caminiti <lorcaminiti <at> gmail.com> writes:

> [...]
>
> * [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 <typename Iter, typename Dist>
  void myadvance(Iter& i, Dist n) {
    auto functions = hana::make_tuple(
      hana::make_pair(is_random_access_iterator<Iter>{}, [&](auto n) {
        i += n;
      }),
      hana::make_pair(is_bidirectional_iterator<Iter>{}, [&](auto n) {
        if(n >= 0) while(n--) ++i;
        else       while(n++) --i;
      }),
      hana::make_pair(is_input_iterator<Iter>{}, [&](auto n) {
        while(n--) ++i;
      })
    );
    auto f = hana::second(*hana::find_if(functions, hana::first));
    f(n);
  }

I could also have used nested if's, but I think the above works just fine too.

> * 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<int, char> v{1, 'x'};
    fusion::count_if(v, [](auto x) {
        std::cout << "foobar";
        return false;
    });

I don't know, and it's hard to tell from the docs [1].

> [...]
>
> * [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