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

Subject: Re: [boost] [CallableTraits] The formal review begins today
From: Barrett Adair (barrettellisadair_at_[hidden])
Date: 2017-04-09 16:50:20

On Sun, Apr 9, 2017 at 10:16 AM, Bruno Dutra via Boost
<boost_at_[hidden]> wrote:
> 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.
>

I went back and forth on this. I decided to consistently *not* handle them,
but I'm pretty convinced that this was the wrong decision. This should be
fixed.

> * 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&.

parent_class_of has this behavior. qualified_parent_class_of attempts to
address the use case of using the class as a function parameter, where foo&
or foo const & would often be preferred over 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 idea of qualified_parent_class_of is to copy cv/ref from the member
function to the class. This doesn't really make sense for PMDs, so maybe
PMDs should cause a substitution error here. parent_class_of can be
used for the 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.
>

I agree, these will be removed -- except for, perhaps, pop_args_front and
push_args_front. These can be useful when "thunking" member functions.

> * 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.
>

Agreed, nice catch.

> * 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.
>

Do you think it would be better to rename foo_t to foo and remove the
"lazy" traits entirely? I came close to doing this for the reasons you
mention above.

>
>>   * 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.
>

Can you elaborate on what you mean by "minimal working example"? I
plan to add more "motivating examples" and comparison code with
Boost.FunctionTypes, however I think the documentation is riddled with
what I would describe as minimal working examples.

>
>>   * Tests
>>
>
> It seems only eager versions of traits are tested, I'd like to see the lazy
> versions tested as well.

Hmm, fair point. This makes me more inclined to remove the lazy versions.
The more that I think about the lazy versions, the more I think they
are useless.

> 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.
>

Actually, I think the test coverage is pretty good here. SFINAE tests can be
found in the *_constraints.cpp files, such as
https://github.com/badair/callable_traits/blob/master/test/arg_at_constraints.cpp

>   * 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
>

Thanks for the thorough and insightful review. This has been very helpful.