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

Subject: [boost] [hana] Lorenzo's Formal review for Hana
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2015-06-22 14:24:25

Hello Glen, Louis, and all,

Please find below my review of the proposed Boost.Hana library.

> - Whether you believe the library should be accepted into Boost

Yes, Boost.Hana should be accepted into Boost.

I decided to make my acceptance vote unconditional.
However, I do have some concerns and I hope these can be addressed if
Hana is in fact accepted into Boost.
I marked these concerns as "[CONCERN]" below.

> - Your name

Lorenzo Caminiti.

> - Your knowledge of the problem domain.

I have been using C++ Template Meta-Programming (TMP) for 10+ years now.
I routinely maintain ~23,000 lines of production code that extensively
uses Boost.MPL, Boost.Fusion, and a TMP-based serialization mechanism
(among a number of other Boost libraries) -- compilation times are a
*huge* issue with this code!

> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
>   * Design

The library API seems reasonable, consistent, and powerful.

* [CONCERN] Assertions should be renamed from ..._CHECK to ..._ASSERT
(to be consistent with assert, static_assert, BOOST_STATIC_ASSERT,
BOOST_MPL_ASSERT, etc.).

* 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.)

* The following is very nice!

    auto has_toString = is_valid([](auto&& obj) ->
decltype(obj.toString()) { });

I've been wanting to do the above at function scope for a long time
(so I can program it within the function where I actually need it).
Now with C++14 generic lambdas we finally can :)

Also this is very nice!

    template <typename T>
    std::string optionalToString(T const& obj) {
      return if_(has_toString(obj),
        [](auto& x) { return x.toString(); },
        [](auto& x) { return "toString not defined"; }
      )(obj);
    }

Somewhat similar to my static_if (renamed call_if, after Vicente's
suggestion) proposal using C++14 generic lambdas:
http://boost.2283326.n4.nabble.com/static-if-Is-there-interest-in-a-static-if-emulation-library-tt4667118.html#none

BTW, how would you program the following possible implementation of
std::advance in Hana?

    template<typename Iter>
    struct is_random_access_iterator : std::is_same<
        typename std::iterator_traits<Iter>::iterator_category,
        std::random_access_iterator_tag
    > {};

    template<typename Iter>
    struct is_bidirectional_iterator : std::is_same<
        typename std::iterator_traits<Iter>::iterator_category,
        std::bidirectional_iterator_tag
    > {};

    template<typename Iter>
    struct is_input_iterator : std::is_same<
        typename std::iterator_traits<Iter>::iterator_category,
        std::input_iterator_tag
    > {};

    template<typename Iter, typename Dist>
    void myadvance(Iter& i, Dist n) {
        Iter *p = &i; // So captures change actual pointed iterator value.
        call_if<is_random_access_iterator<Iter> >(
            std::bind([] (auto p, auto n) {
                *p += n;
            }, p, n)
        ).template else_if<is_bidirectional_iterator<Iter> >(
            std::bind([] (auto p, auto n) {
                if(n >= 0) while(n--) ++*p;
                else while(n++) --*p;
            }, p, n)
        ).template else_if<is_input_iterator<Iter> >(
            std::bind([] (auto p, auto n) {
                while(n--) ++*p;
            }, p, n)
        ).else_(
            std::bind([] (auto false_) {
                static_assert(false_, "requires at least input iterator");
            }, std::false_type()) // Use constexpr value.
        );
    }

* 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?
For example, the following is supported, correct? (It should be
because these can use variadic macros...)

    struct Person {
      BOOST_HANA_DEFINE_STRUCT(Person,
        (std::map<int, char>, name),
        (int, age)
      );
    };

P.S. These sort of macros /could/ be implemented as follows if users
found this syntax to be more readable (but maybe not...):

    struct Person {
      BOOST_HANA_DEFINE_STRUCT(Person,
        (std::map<int, char>) name,
        (int) age,
      );
    };

* [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?

* [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?
Do MPL and Fusion relay on a similar assumption? Yes/no, why...

* [CONCERN] I would prefer a descriptive name for the library instead of Hana.

I am well aware that Phoenix, Spirit, Wave, etc. are examples of
non-descriptive names in Boost. I consider those bad naming choices
and I would not follow them.
Changing the library name should only take a few days (maybe 2-5
days... especially with tools like grep and sed) so that should not be
a problem.
Others pointed out that there might be copyright/trademark issues with
the name "Hana". The possibility to have to change such name at some
point /after/ the library has been released in Boost would be a
disaster for the users... such risk should be avoided.

Possible names?
    typeval (given the library is about "TYPEs as VALuses", plus the
similar, already familiar, name std::declval)
    meta
    metaval
    metaexpr
    generic (assuming Matt Calabrese is no longer planning to use this)
    ...

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

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

>   * Implementation

Some code can be optimized, but the author already mentioned he will
look into that this summer.
I did not see any specific issue otherwise.

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

* 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()) { });

* 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));
    });

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

* Why the "Runtime performance of reverse on normal container" for
fusion::vector stops at "Number of elements" = 50?

>   * Tests
>
>   * Usefulness

* If not for anything else, I will use this library for its SFINAE facilities.
Second, I will use this library in production code for its improved
compilation times (assuming these can be confirmed on MSVC).
Finally, I am curious to use this library in general because it seems
it will improve my meta-programming, also making easier to read.

* [CONCERN] The author should be committed to support the library on
GCC and MSVC once that becomes feasible, even if that requires a good
deal of workarounds.
For example, Boost.Preprocessor is full of MSVC workaround (because
MSVC preprocessor is still not standard compliant... after 30+ years
of existence of the C preprocessor).
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.
I do hope that adoption into Boost will push vendors to support C++14
constructs used by this library TMP techniques.
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.

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...

> - 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

On Wed, Jun 10, 2015 at 2: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