Re: [boost] [CallableTraits] The formal review begins today

Subject: Re: [boost] [CallableTraits] The formal review begins today
From: Bruno Dutra (brunocodutra_at_[hidden])
Date: 2017-04-09 15:16:43

On Mon, Apr 3, 2017 at 8:45 AM, Louis Dionne via Boost <
boost_at_[hidden]> wrote:

> The formal review of Barrett Adair's CallableTraits library begins today,
> April 3rd, and ends on April 12th.
>
> We encourage your participation in this review. At a minimum, kindly state:
> - Whether you believe the library should be accepted into Boost, and
>

Yes.

>   conditions for acceptance if any
> - Your name
>

Bruno Dutra

> - Your knowledge of the problem domain
>

I've done extensive work with template metaprogramming for the past couple
of years while developing Metal.

> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
>   * Design
>

The design is sound overall, but I have a couple of remarks that I'd like
to see addressed.

* I find it very surprising that the top-level cv and ref qualifiers aren't
consistently handled throughout, there is no reason why querying a property
of a member function through a PMF should depend on the qualifiers of the
pointer itself.

* Why does qualified_parent_class_of return a ref qualified type even for
unqualified PMFs? This is again surprising, I would
expect qualified_parent_class_of_t<void (foo::*)()> to return foo, not
foo&.

* Why does qualified_parent_class_of always return a const& qualified type
for PMDs? I can imagine that this would be quite an annoyance if the user
wants to apply custom qualifiers to it for one reason or another,
especially rvalue reference, which would be silently collapsed away by the
lvalue reference. The safest choice IMO would be to return an unqualified
type.

* The various algorithms on sequences feel out of place in this library,
especially considering there are better and more generic options available
in Boost and even in the standard library for the arguably most useful
ones, namely std::tuple_element and std::tuple_size.

* The traits args and expand_args would be better merged into a single
binary trait, whose second template template parameter is defaulted to
std::tuple.

* The documentation explicitly states that the violation of pre-conditions
for transformation traits produces a SFINAE-friendly substitution error.
While that does seem to be true for their eager *_t versions as fair as I
could verify, it can't possibly hold true for the lazy versions of
transformation traits, which is a direct consequence of the way they are
defined, namely

template<typename T>
struct trait {
    using type = trait_t<T>;
};

This always produces a hard error if the pre-conditions for trait_t are
violated.

One option would be to clearly state in the documentation to what extent
transformation traits are SFINAE-friendly, however I deem it surprising
that the eager versions should be SFINAE friendly whereas lazy versions are
not, therefore I suggest that the definition of lazy transformation traits
be amended to guarantee this property as well, possibly by simply
explicitly triggering a substitution error like follows

template<typename T, typename = void>
struct trait {};

template<typename T>
struct trait<T, std::void_t<trait_t<T>>> {
    using type = trait_t<T>;
};

* The above is also true for predicate traits, which define a nested
typename type in terms of an unfriendly expression, instead of inheriting
it along with the integral constant value. This is less of a problem in
this case, since the user is most interested in the nested integral
constant value, but still there doesn't seem to be any reason for the
nested typename type to not be SFINAE-friendly.

>   * Implementation
>

Quickly skimmed through, looks fine.

>   * Documentation
>

I missed a Quick Start section that helps the user set up a minimal working
example, even if that would be trivial in this case.

>   * Tests
>

It seems only eager versions of traits are tested, I'd like to see the lazy
versions tested as well.
Also I didn't find tests that ensure SFINAE-friendly substitution errors
are produced instead of hard errors, those should be added since this
behavior is emphasized.

  * Usefulness
> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
>

Yes, on Linux using GCC trunk and also Clang trunk + libc++ trunk.

>   * What was the experience? Any problems?
>

Pleasant, no issues.

> - How much effort did you put into your evaluation of the review?
>

Spent about 3-4 hours reading the documentation and playing with the
library.

Bruno