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

Subject: Re: [boost] [Hana] Formal review for Hana
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2015-06-16 13:43:06

Le 14/06/15 21:19, Louis Dionne a écrit :
> Paul Fultz II <pfultz2 <at> yahoo.com> writes:
>
>
>> - `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.
Meta uses it also. I'm wondering if reverse_fold shouldn't accept the 
same function signature as fold. It shouldn't be the case for fold_left 
and fold_right, as the parameters of the function to apply are exchanged.

fold,fold.left:F(T)×S×(S×T→S)→S
fold.right:F(T)×S×(T×S→S)→S
BTW, it would be nice if reverse_fold (and any function) had its own 
signature.
reverse_fold:F(T)×S×(S×T→S)→S

>
>> - 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 would prefer of Hana uses only CamelCase for C++17/20 Concepts or for 
C++14 type requirements. This will be confusing for more than one.

You have also the option to define make having a class template as 
parameter (See [A]).

     make<_tuple>(xs...)
     to<_tuple>(xs)

On Expected I defined it with a default parameter and specialize it
template <class T = holder, class E = exception_ptr>
struct expected. This allowed to use

     make<expected<>>(xs)

I don't know how this trick could be applicable to variadic templates.
> 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.
I would prefer also that Hana defines its concrete types, but I think 
this battle is lost.
>> - 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.
This is capital to the understanding of Hana. Hana has no automatic 
mapping. You must state explicitly that a type is a model of a Concept 
and I like it. This doesn't follow however the current trend of C++17/20 
Concepts.

A type is a model of a Concept is it has an explicit mapping of its 
associated mapping structure.

The Constant concept could have a mcd that defaults to the nested 
members, and so  make easier your mapping.

I would prefer if Hana used a different name for his Concepts. In 
addition Hana Concepts have an associated class, which C++7/20 Concepts 
are predicates.
I would reserve the CamelCase for C++ Concepts and the lowercase for the 
mapping struct,

     struct applicative {
         template <typename A>
         struct transform_impl;
     };

     template <typename F>
     concept bool Applicative = requires ....;

> - 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.
I agree that following the same formalism than the C++ standard will be 
nice.

An alternative would be to use C++TS Concepts for describing them :)
>
>> - 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 don't know. The constraints must be stated where needed. There is no 
reason a concept shouldn't require that the underlying type is a model 
of some other Concept it this is needed. I believe that all the concepts 
make use of MoveConstructible types, or am I wrong. I agree that the 
documentation is not precise enough respect to this point.
>> - 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].
>
I'm not a fan of having all the algorithms on the same level. This force 
us to choose a different name. Haskell has this constraint and uses 
prefix f, m a , but we have namespaces in C++. I would move all the 
algorithms to a namespace associated to the concept. This would give us 
much more flexibility respect to the names. I know already that no one 
share this design but I wanted to shere it again.

namespace monoid {
     struct mapping // monoid::mapping plays the current use of Monoid
     {
         template <class T> struct instance {...}; // global mapping
         // mcd
         // as_additive  (T;+; 0)
         // as_multiplicative (T;*; 1)
         // as_sequence (S(T); append; T{})
     };
     // operations
     // ...
}

This doesn't mean that we can not have an index of all the algorithms.
>> - 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.
Concat could be defined on a DiscontinuousRange, but Hana doesn't have 
this Concept.
> 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.
I missed the fact that Monoid introduces + for plus. The operator+ must 
be documented and appear on the Monoid folder.

I wouldn't be weird to use + for concatenating strings as std::string 
provides already operator+(). It would be much more weir to use 
zero/plus/+ with a monoid (int, *, 1)

This is the problem of naming the function associated to a Concept.

Haskell uses mappend and mempty? These names comes from the List Monoid. 
I don't like them neither.
IMHO, the names of Monoid operations must be independent of the domain.
A monoid is a triplet (T, op, neutral), where op is a binary operation 
on T and neutral is the neutral element T respect to this operation. 
What is wrong then with monoid::op and monoid::neutral, instead of the 
Hana globals plus and zero? Too verbose? Having these names on a 
namespace let the user make the choice.

I would not provide any operators for the Monoid Concept.

> 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.
With the previous Hana version we had structs defining different Minimal 
Complete Definition (mcd). One of the possible mcd would be one that 
just forwards using some syntactical convention. Thus doing the mapping 
for this kind of types consists only in making the mapping inherit from 
this mcd.

>> - 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).
I'm not aware of ODR issues with variable templates. I will be 
interested on a pointer.
> 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 best is to measure it ;-) Are there any measures of the same program 
using Hana and Meta?
>
>> 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).
I would like that more compilers can run Hana programs without problems, 
but this is the compiler problem.
Please, let Hana make use of as many useful features for the library as 
the C++14 language has.

I would be even accept some extensions that even if not already in the 
standard or on a TS, have a on going proposal. Of course this should be 
provided conditionally. As for example compile time string literals.
>
> [1]:https://github.com/ldionne/hana/issues/114
> [2]:https://gist.github.com/ldionne/32f61a7661d219ca834d#file-main-cpp
> [3]:http://ruby-doc.org/core-1.9.3/Integer.html#method-i-times
> [4]:https://github.com/ldionne/hana/issues/100
> [5]:https://github.com/ldionne/hana/issues/115
> [6]:https://github.com/ldionne/hana/issues/116
> [7]:https://github.com/ldionne/hana/issues/82
> [8]:https://github.com/ldionne/hana/issues/117
> [9]:http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_active.html#2104
> [10]:https://github.com/ldionne/hana/issues/76
>
>
[A] https://github.com/viboes/std-make/tree/master/doc/proposal/factories