[boost] [Hana] David Stone's formal review

Subject: [boost] [Hana] David Stone's formal review
From: David Stone (david_at_[hidden])
Date: 2015-06-25 02:00:09

- Whether you believe the library should be accepted into Boost

Yes

- Your name
>

David Stone

> - Your knowledge of the problem domain.
>
Advanced

> - What is your evaluation of the library's:
>   * Design

Good.

It seems like most of the library follows the naming conventions of boost
and the standard library (identifiers_like_this), but there are a few
exceptions. The most notable is boost::hana::IntegralConstant. Why not
integral_constant?

Aside from minor issues like this, I am a huge fan of the overall idea of
making compile-time and run-time code the same. Ideally we would have just
a single language in C++ (as opposed to plain C++, template C++, constexpr
C++, macro C++, and concept C++) and a single style of programming. The
design of this library follows and extends this general trend in C++ that
started with constexpr.

>   * Implementation
>

Looking through the source, it seems to flow naturally. Most of the code
looks essentially the same as I would have written it. It has small,
self-contained functions and appropriate use of macros, comments, and
static_asserts.

It would be nice if your IntegralConstant provided a specialization of
std::numeric_limits. This would allow it to work properly with a few
libraries (two examples are my bounded::integer library and this fixed
point library: https://github.com/mizvekov/fp). You could then detect
type-level compile-time constants as being any integer type for which
std::numeric_limits<T>::min() == std::numeric_limits<T>::max(), which would
allow the interoperability to flow the other way, but I do not know if the
library ever requires this information or if it just relies on the compiler
to complain about things that are not constexpr.

>   * Documentation
>

Very good from some perspectives. I started using the library by trying out
checking expression validity, and it followed a logical progression,
covering all types of members.

This naturally led to me wondering if it is possible to use the library to
test for the existence of a member named `blah`, but without knowing what
category of member it is (variable, function, type, or template). I am
assuming that this is as simple as saying `is_valid(variable) or
is_valid(function) or ...`

However, I feel that the documentation is trying to serve two incompatible
purposes. On the one hand, it is trying to explain the theory of type
computation using terms from functional programming. On the other hand, it
also trying to be a practical guide to using the library. I feel that the
second goal is compromised by the first. It seems that most of the content
needed for the "cut to the chase" user is present, but by having this
presentation in the form of a single document dilutes it.

In other words, I don't know that the documentation passes the coworker
test. If I were to show it to my coworkers, I can already hear them saying
"All this stuff about computational quadrants and heterogeneous algorithms
is all well and good, but how do I actually use this thing?".

>From the perspective of someone who has never used MPL or Fusion (I started
metaprogramming in C++11 and C++14 and never found a need for them), I do
not have a problem with the comparisons to the other libraries in the
documentation. I know another reviewer had mentioned that as a concern, but
it just reads as evaluating prior art rather than reading as assuming prior
knowledge.

  * Tests
>

I have not looked at the library's tests, I just tried to use it.

>   * Usefulness
>

The lack of constexpr lambdas really hurts the library's usefulness. It is
because of this hole in the language that it can be difficult to slowly
move to hana. I would like to just replace the implementation of a few
functions with hana implementations at first, but then my functions are no
longer constexpr if I do the obvious implementation. I have to make sure to
change all of my types to IntegralConstant, for instance, to get a
non-constexpr value from which I can extract a constexpr value from its
type. This has a larger ripple throughout than I would like for just
testing out a library before committing to it.

That being said, once updating your code to fit into this paradigm, it does
seem to simplify things a bit. I expect that as I update my library to be
more hana friendly throughout, this simplification will increase. It was a
bit disheartening at first when my code size grew slightly as I wrote
various shims to translate to the correct types (I had more constexpr
functions returning bool, and not so much classes that derive from
integral_constant), but that ended up being a temporary state until I
worked through more code.

I wonder if I passed a constexpr function object rather than a
non-constexpr lambda if these limitations would be removed? In practice
this would probably lead to more code in the intermediate step even if it
did work due to the boilerplate of defining a non-lambda function object.

> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
>
  * What was the experience? Any problems?
>

clang 3.6.0

The first thing I tried was to use is_valid to determine the existence of a
member type. This immediately crashed clang in the parser (I haven't had
time to extract the bug report yet, but I will). It didn't seem to be the
library directly, especially since others have used this feature, but the
combination of my library (bounded::integer) with this feature of hana
seemed to interact to cause a crash. clang seemed to perform a bit better
with my second test, which was to replace some enable_if with if_.

> - How much effort did you put into your evaluation of the review?
>
> In addition to talking with Louis at C++Now, I spent a few days trying out
the library.