Re: [boost] [Hana] Informal review request

Subject: Re: [boost] [Hana] Informal review request
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2015-03-05 14:02:33

Steven Watanabe <watanabesj <at> gmail.com> writes:

> 
> AMDG
> 
> I've just glanced over the main page of the documentation
> and I had a few comments:
> 
> - _tuple<Person, Car, City>
>   Historically names beginning with _ are used for placeholders.
>   i.e. in Boost.Bind/Lambda/Phoenix/Parameter.  Is there
>   a good reason not to call it tuple?

It is because of other constructs in Hana like `tuple_t<Types...>` 
which are variable templates and whose type is not specified. I have 
not found a good naming convention for variable templates and types yet. 
I'll sketch out the current state of affairs, and perhaps someone can 
suggest a convention that would work well.

Hana currently proposes the following:

_tuple<T...> : 
    The type of a tuple containing objects of type T...

make_tuple(x...): 
    Create a tuple of type _tuple<decay_t<decltype(x)>...>

tuple(x...):
    Currently equivalent to make_tuple(x...), but deprecated 
    because I'd like to use it in place of _tuple.

Tuple:            
    The generalized data type of tuples.

tuple_t<T...>: 
    A variable template returning a tuple containing types.
    This object has an _unspecified type_, but its data type is 
    Tuple. In other words, this behaves as a tuple, but it does 
    not have the same representation. It's for optimization 
    purposes. Where it becomes really tricky is that the type 
    of this object is dependent, so it can't be used for pattern 
    matching. However, it is currently specified as inheriting 
    the _tuple_t<T...> type, which allows one to pattern match 
    in overloaded functions.

tuple_c<T, v...>:
    A variable template returning a tuple containing integral 
    constants. This returns an object of type _tuple_c<T, v...>, 
    which is not the same as _tuple<something...>. It behaves 
    like a tuple, but has a different representation. 
    For optimization purposes too.

Other components follow similar conventions. For example, 

_range<T, from, to>:
    The type of a range containing IntegralConstants of 
    data type T spanning the given interval.

Range:
    The generalized data type of _range<...>.

make_range(from, to):
    Create a _range<common data type(from,to), from, to>.

range(from, to):
    Equivalent to make_range(from, to), but deprecated because 
    I'd like it to replace `_range`.

range_c<T, from, to>:
    Shortcut to create a range of IntegralConstants.
    This is similar to vector_c in the MPL.

And so it goes. Writing this, perhaps it would make sense to 
simply replace `_range` by `range`, `_tuple` by `tuple` and etc.

> - // BOOST_HANA_RUNTIME_CHECK is equivalent to assert
>   In that case, what is the point of using it instead of assert?
>   I think you should probably avoid using these constructs
>   in the user documentation of other parts of the library.
>   It's just a distraction, so it's better to rely on
>   standard components, which users probably already
>   understand.

The problem is that `assert` is not define when NDEBUG is defined, 
but I want my tests to fire up all the time. Since these examples 
are Doxygen'd out of actual source files, the macros end up in the 
documentation. Basically, If I could redefine `assert` to be 
`BOOST_HANA_RUNTIME_CHECK` in those test files, I'd be really 
happy. Would it be against good software craftsmanship to write

    ... include all you need ...
    
    #ifndef assert
    #   define assert(...) BOOST_HANA_RUNTIME_CHECK(__VA_ARGS__)
    #endif

> - Is there a reason for using length instead of size,
>   which would be more idiomatic in C++?

Oversight caused by personal preference. 
I guess I should change that.

> - Amphibian algorithms!!!???  Okay, I guess the
>   name is kind of clever.  I can't say that I
>   really like it, though.

If you have anything better to propose, I'm all ears.

> - foldl/foldr.  I really hate these names.  I'd prefer
>   fold/reverse_fold, but even fold_left/fold_right
>   would be better.

I personnally hate reverse_fold, since I don't see how it is a 
"reverse" fold. It's just a fold with different _associativity_ 
properties. As for fold_right and fold_left, I guess it comes
down to personal preference. I opened a "poll" on GitHub to 
settle this. See [1].

> - count is the equivalent of std::count_if, not std::count.
> - find/lookup should be find_if/find

You're right. It's sometimes hard to set aside personal
preferences for the greater good, but I'll do it :-).

> - In general I would expect any algorithm that takes
>   a single element for comparison to have both
>   f and f_if forms.

It is possible that a structure can do lookup by equality but not 
by any predicate, so it wouldn't be able to provide both. But do 
you have an example in mind of a place where there's an *_if 
variant missing in Hana? It would almost surely be an oversight.

> - sort(predicate): shouldn't this be sort(sequence)?

Typo; thanks for reporting.

Louis

[1]: https://github.com/ldionne/hana/issues/18