Re: [boost] Boost Hana comments

Subject: Re: [boost] Boost Hana comments
From: Louis Dionne (ldionne.2_at_[hidden])
Date: 2014-08-18 16:12:02

Sorry for the slow answer; I completely missed your message. Also note that
I've been working on a large changeset for the past week, so some comments 
are actually already fixed in my local version.

Jeremy Maitin-Shepard <jeremy <at> jeremyms.com> writes:

> 
> First off, while I haven't thoroughly reviewed the library, I am certainly
> impressed by a lot of what I see.  It is obvious that this library
> represents a lot of excellent work.
> 
> Here are my miscellaneous comments:
> 
> Constant type class:
> --------------------
> 
> Why isn't std::integral_constant an instance?

Good observations; that's a matter of time. std::integral_constant is an 
instance in my local version.

> There is no documentation of what the value method actually returns, just
> that it returns something that is a constexpr.  Does
> value(integral<int,3>))
> return int, or does it return integral<int,3> ?

It returns the underlying value of `integral<int, 3>`, which is an `int`. 
The documentation will be improved to explain it.

> Perhaps the Constant type class should be "parameterized" by a type
> specifying the return type of value.

Might be a good idea; I'll think about how this would interact with the
rest of the library.

> Integral data type:
> -------------------
> 
> It would be better not to duplicate std::integral_constant.  However, I
> recognize that the overloaded operators are useful and you wouldn't be
> able
> to do this with std::integral_constant (well, I suppose you still could,
> but
> as they wouldn't be found by ADL, they wouldn't be terribly useful).  This
> rationale (assuming it is correct) should be documented though.

This is part of the rationale. There's also the fact that integral_constant 
requires including <type_traits>, which is about the size of the whole
library; 
I find this annoying. I'll document that.

> Logical type class:
> -------------------
> 
> What is really gained by making this a type class at all?

Yes; how would you branch at compile-time otherwise? The if_ method for e.g. 
Integrals is not equivalent to a normal C++ if-statement; it allows both 
branches to have different and incompatible types. Also, since we want to
be able to branch on `integral`s but also on `mpl::integral_c`s, we need a 
type class. 

> It seems that and_ and or_ require that all arguments be of the same "hana
> datatype", though actually this isn't documented anywhere and this might
> not
> even be the precise requirement (but obviously it needs to be documented
> very explicitly, and this probably applies to almost every method in
> Hana).
> 
> Really we really should be able to mix multiple data types in a call to
> or_
> or and_, though.  Otherwise the usability is unreasonably limited.  Of
> course there is then the question of return type.  However, I would assume
> most users would be happy getting back either a Hana Integral or a bool,
> depending on whether it is a compile-time or runtime condition.  Certainly
> this would be better than getting back a compiler error.

The arguments of `and_` and `or_` only need to be Logicals. I'll document
it.

> Another question: how does the performance of and_ and or_ compare to the
> optimized verions you showed in your MPL11 talk?

I haven't benchmarked that yet, but I expect it to be significantly less 
efficient. I'll have to think about a way of specifying the requirements
of `Logical` in a way that makes optimizations possible, which is not the
case currently.

> Pair datatype:
> --------------
> 
> Why duplicate std::pair?

For the same reason as `std::integral_constant`; operators.

> Alternatively, why duplicate list?
>
> Perhaps list should just define first and second as well?
>
> Product type class:
> ----------------
> 
> Why isn't a 2-element List, a 2-element std::tuple, etc. an instance?

`hana::list`, `std::tuple` and friends can't be made an instance of the 
`Product` type class because they would not satisfy its laws.

> List type class:
> ----------------
> 
> Just as for Logical, it would be useful to be able to mix datatypes in
> calls
> to concat, zip, zip_with.

I agree, but it would then be harder to implement those operations
efficiently, 
since you would not control the representation of all the arguments. I'll 
think about a way of supporting different data types while still allowing
optimizations.

> The list function itself shouldn't be documented as part of the type
> class,
> since it isn't logically part of  it.

That's because `List` was both a data type and a type class. In my local 
version, `List` is only a type class and the data type is named `Tuple` 
instead, which makes this a non-issue.

> What about get<N>, get<N,M> accessors?  I understand these can be defined
> in
> terms of the other methods, but clearly they are useful on their own.

For `get<N>`, I'll provide an helper method `at_c<N>(iterable)`. I don't 
understand what you mean by `get<N, M>` though. What does `get<N, M>` do?

Regards,
Louis

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