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

Subject: Re: [boost] [Hana] Formal review for Hana
From: Paul Fultz II (pfultz2_at_[hidden])
Date: 2015-06-15 12:49:11

> > Here some points I see as I have been reviewing Hana. I see you have made
> > some changes on github, so maybe some of this doesn't apply anymore.
> 
> The changes I made are only to develop, so whatever you say is still
> applicable to master.
> 
> 
> > - Dot shouldn't be used in names, underscores should be used instead.
> 
> This will be modified, since it was asked for by 3 different persons so
> far.
> For future reference, you can refer to [1] for the status of this issue.

Awesome.

> 
> 
> > - `fold` and `reverse_fold` should be preferred over `fold_right` and
> >   ``fold_left`. This is more familiar to C++ programmers.
> 
> First, to my knowledge, the only libraries that even define fold and/or
> reverse_fold are Fusion and MPL, so it's not like there was an undeniable
> precedent for using those names instead of something else in C++. But
> even then, `fold` and `reverse_fold` functions are provided for
> consistency
> with those libraries, so I really don't see what's the problem. If you
> prefer those names, you can use them and they have exactly the same
> semantics as their Fusion counterpart.
> 
> 
> > - Concepts are capitalized, however, models of a concept should not be
> >   capitalized(such as `IntregralConstant`, `Either`, `Lazy`, `Optional`,
> >   `Tuple`, etc)
> 
> `IntegralConstant`, `Tuple`, etc... are tags used for tag dispatching,
> like Fusion's `vector_tag` & friends. I can't use a non-capitalized
> version as-is, because it's going to clash with `integral_constant`.
> Also, I find that using something like `tuple_tag` is uglier than using
> `Tuple`. Consider for example
> 
>     make<tuple_tag>(xs...)
>     to<tuple_tag>(xs)
> 
>     make<Tuple>(xs...)
>     to<Tuple>(xs)
> 
> I find that using `Tuple` just leads to prettier code. Also, since Hana
> does not specify the type of all of its containers, we refer to them by
> their
> tag instead of their (unspecified) type. So for example, I talk of a Hana
> `Range`, not a Hana `range`, because the latter is not a type. If I was to
> use the name `range_tag` instead of `Range`, I couldn't do that as easily.
> Fusion does not have this problem because all of its containers have well
> specified types, so you can always talk about a `fusion::vector` and it is
> always understood that this includes `fusion::vector3`, but Hana does not
> have that.

Actually, for me, seeing it written as `tuple_tag` makes so much more sense
when I read the code, and could help a lot when I read the documentation.
Perhaps, that is just me, because other people don't seem to struggle with
this.

> 
> 
> > - IntregralConstant is very strange. In Hana, its not a concept(even
> though
> >   its capitalized), but rather a so called "data type". Furthermore,
> because
> >   of this strangeness it doesn't interoperate with other
> >   IntregralConstants(such as from Tick) even though all the operators
> are
> >   defined.
> 
> IntegralConstant is not a concept, that's true. The fact that it does not
> interoperate with Tick's IntegralConstants out-of-the-box has nothing to
> do with that, however. You must make your Tick integral constants a model
> of Hana's Constant concept for it to work. See the gist at [2] for how to
> do that.

Awesome, thanks. So all the concepts are explicit.

> 
> 
> > - The `.times` seems strange and should be a free function so it works
> with
> >   any IntregralConstant.
> 
> The idea of having a `.times` member function comes from Ruby [3].
> I personally think it is expressive and a simple tool for a simple job.
> The syntax is also much cleaner than using a non-member function. Consider
> 
>     int_<10>.times([]{ std::cout << "foo" << std::endl; });
>     times(int_<10>, []{ std::cout << "foo" << std::endl; });
> 
> However, as we've been discussing in this issue [4], I might add an
> equivalent non-member function.

If you find the chaining much cleaner, then perhaps you could make it a
pipable function instead:

    int_<10> | times([]{ std::cout << "foo" << std::endl; });

This way it could still apply to any IntegralConstant.

> 
> 
> > - In the section 'Taking control of SFINAE', seems like it could be
> solved a
> >   lot simpler and easier using `overload_linear`.
> 
> First, `overload_linearly` is an implementation detail, since I've moved
> the Functional module out of the way to give me the possibility of using
> Fit in the future. However, your point is valid; I could also write the
> following instead:
> 
>     auto optionalToString = hana::overload_linearly(
>         [](auto&& x) -> decltype(x.toString()) { return x.toString(); },
>         [](auto&&) -> std::string { return "toString not defined"; }
>     );
> 
> This approach is fine for the optionalToString function, which is rather
> simple. I wanted to show how to use Optional to control compile-time
> empty-ness in complex cases, so I'll just expand this section or change
> the example to something that is really better solved using Optional.
> 
> Thanks for the heads up; you can refer to this issue [5] in the future.

Ok got it. Although, SFINAE is a pretty powerful compile-time Optional built
into the language.

> 
> 
> > - Concepts refer to 'superclasses' these should be listed either as
> >   refinements or listed under the requirements section(which seem to be
> >   missing). It would be nicer if the concepts were documented like how
> they
> >   are at cppreference: http://en.cppreference.com/w/cpp/concept
> 
> This was fixed on develop. I now use the term "Refined concept" instead of
> "Superclass". Regarding concept requirements, they are listed in the
> "minimal complete definition" section of each concept. Then, semantic
> properties that must be satisfied are explained in the "laws" section.
> 

Great.

> 
> > - Concepts make no mention of minimum type requirement such as
> >   MoveConstructible.
> 
> I believe the right place to put this would be in the documentation of
> concrete models like `Tuple`, but not in the concepts (like `Sequence`).
> Hana's concepts operate at a slightly higher level and they do not really
> have a notion of storage. But I agree that it is necessary to document
> these requirements. Please refer to this issue [6] for status.

I was thinking more when using algorithms, since tuple will take on the
constructibility of its members.

> 
> 
> > - Organization of documentation could be better. Its nice showing what
> >   algorithms when the user views a concept, but it would be better if
> all
> >   the algorithms could be viewed together.
> 
> I assume you are talking about the reference documentation and not the
> tutorial. I agree that it could be easier to look for algorithms. There
> are also other quirks I'd like to see gone. The problem is that Doxygen
> is pretty inflexible, and I'm already tweaking it quite heavily. I'm
> considering either using a different tool completely or making some
> changes in the organization of the reference.
> 
> Your more precise comment about viewing algorithms on their own page is
> already tracked by this issue [7].

Awesome. Have you thought about using a different documentation tool
instead,
like mkdocs or sphinx?

> 
> 
> > - For compile-time `Iterable` sequence(which is all you support right
> now),
> >   `is_empty` can be inferred, and should be optional.
> 
> How can it be inferred?

Well, there is several ways it could be formailised, but either `head` and
`tail` do not exist for an empty sequence, or if `tail` always returns an
empty sequence even when empty, you just detect that `seq == tail(seq)`.

> 
> 
> > - Overall, I think the Concepts could be simplified. They seem to be too
> >   complicated, and it leads to many surprises which seem to not make
> >   sense(such as using `Range` or `String` with `concat` or using
> >   `tick::integral_constant`).
> 
> 1. Concatenating ranges does not make sense. A Hana Range is a contiguous
>    sequence of compile-time integers. What happens when you concatenate
>    `make_range(0_c, 3_c)` with `make_range(6_c, 10_c)`? It's not
> contiguous
>    anymore, so it's not a Range anymore.

Even though concat takes a Range, why can't it just return a tuple instead?

> 
> 2. Concatenating strings makes complete sense, indeed. This could be
> handled
>    very naturally by defining a `Monoid` model, but it was not done
> because
>    I did not like using `+` for concatenating strings :-). I opened this
>    issue [8] to try and find a proper resolution.
> 
> 3. Tick's integral_constants can be handled as I explained in the
>    gist at [2].
> 
> As a fundamental library, Hana was designed to be very general and
> extensible
> in ways I couldn't possibly foresee. Hence, I could have stuck with
> Fusion's
> concept hierarchy (minus iterators), but that would have been less general
> than what I was aiming for. Also, Hana is slightly biased towards
> functional
> programming, and it reflects in the concepts. If that is what you mean by
> "complicated", then I'd say this generality and power is a feature rather
> than a bug.
> 
> I would really like to know specifically which concepts you find too
> complicated or superfluous. There are definitely things that could be
> improved, but in general I am very content with the current hierarchy
> and I think this is one of Hana's strengths, to be frank.

I agree that using Fusion style of concepts is a bad idea. The
representation
of the machine doesn't map to the representation in the compiler.

I think part of it is my confusion with data types and concepts when reading
the documentation.

Also, it is more general, and the more I think about it, I don't think there
is a simpler way and still support compile-time lazy and infinite sequences
and be efficient.

> 
> 
> > - Currently, none of the algorithms are constrained, instead it uses
> >   `static_assert`, which I think is bad for a library that is targeting
> >   modern compilers.
> 
> People have mixed opinions about this. I personally think the last thing
> you
> want is for an overload to SFINAE-out because of some failure deep down
> the
> call chain, considering we're working with heterogeneous objects. I think
> the best way to go is to fail very fast and very explicitly with a nice
> static_assert message, which is what Hana tries very hard to do.
> 
> I also think the minority of people that would benefit from having SFINAE
> friendly algorithms is largely outweighted by the majority of non template
> metaprogramming gurus (likely not reading this list) who would rather have
> a nice and helpful `static_assert` message explaining what they messed up.
> 
> Also, there's the problem that being SFINAE-friendly could hurt
> compile-time
> performance, because everytime you call an algorithm we'd have to check
> whether it's going to compile. However, because we're working with
> dependent
> types, checking whether the algorithm compiles requires doing the
> algorithm
> itself, which is in general costly. 

Templates are memoized by the compiler, so the algorithm isn't done twice.

> We could however emulate this by using
> the Tag system. For example, `fold_left` could be defined as:
> 
>     template <typename Xs, typename State, typename F,
>         typename = std::enable_if_t<models&lt;Foldable, Xs>()>
>     >
>     constexpr decltype(auto) fold_left(Xs&& xs, State&& state, F&& f) {
>         // ...
>     }
> 
> This would give us an approximative SFINAE-friendliness, but like I said
> above I think the best approach is to fail loud and fast.

It would fail loud not fast. Using substitution failure, the compiler will
stop substitution as soon as their is a failure, whereas with a
static_assert,
it substitutes everything and then checks for failure. So using enable_if is
always faster than static_assert.

Also, as a side note, you should never use `enable_if_t`, as it is harder
for
the compiler to give a good diagnostic(a macro still works really well
though
if you don't mind macros).

> 
> 
> > - It would be nice if the use of variable templates would be
> optional(and
> >   not used internally), since without inline variables, it can lead to
> ODR
> >   violations and executable bloat.
> 
> Without variable templates, we would have to write `type<T>{}`,
> `int_<1>{}`,
> etc.. all of the time instead of `type<T>` and `int_<1>`. Sure, that's
> just
> two characters, but considering you couldn't even rely on what is
> `type<T>`
> (if it were a type, since `decltype(type<T>)` is currently unspecified),
> we're really not gaining much. In short; no variable templates means a
> less
> usable library, without much benefits (see next paragraph).

How is it less usable? It seems like it would be more usable, since the
library can now support compilers with no or flaky variable templates.

> 
> Regarding variable templates and ODR, I thought variable templates
> couldn't
> lead to ODR violations? I know global function objects (even constexpr)
> can
> lead to ODR violations, but I wasn't aware about the problem for variable
> templates. I would appreciate if you could show me where the problem lies
> more specifically. Also, for reference, there's a defect report [9]
> related
> to global constexpr objects, and an issue tracking this problem here [10].
> 
> Finally, regarding executable bloat, we're talking about stateless
> constexpr
> objects here. At worst, we're talking 1 byte per object. At best (and most
> likely), we're talking about 0 bytes because of link time optimizations.
> Otherwise, I could also give internal linkage to the global objects and
> they
> would probably be optimized away by the compiler itself, without even
> requiring LTO. Am I dreaming?

The size of the symbol table is usually larger than 1 byte for binary
formats.

> 
> 
> > Overall, I would like to see Hana compile on more compilers before it
> gets
> > accepted into boost(currently it doesn't even compile on my macbook).
> 
> What compiler did you try to compile it with? Also, an important GCC bug
> that was preventing Hana from working properly on GCC was fixed a couple
> of days ago, so I'm going to try to finish the port ASAP and I'm fairly
> confident that it should work on GCC 5.2.

Using Apple's clang 6, which corresponds to clang 3.5 off of the trunk. What
is the bug preventing compilation on gcc 5.2?

Paul

--
View this message in context: http://boost.2283326.n4.nabble.com/Boost-Hana-Formal-review-for-Hana-tp4676969p4677133.html
Sent from the Boost - Dev mailing list archive at Nabble.com.