Quantcast

[CallableTraits] The formal review begins today

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Louis Dionne
I don't think I've participated on this list enough to do a formal
review, but here are some comments from a first look:

[...]
Thanks for the feedback so far. Please do consider submitting a formal review; it does
not matter whether you've participated a lot on this mailing list, so long as you have
valuable feedback and are making sound technical points. It will make my decision
as the review manager easier if you submit a formal review than if you just give
informal (yet very valuable) feedback.

Regards,
Louis
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Wed, Apr 5, 2017 at 6:12 PM, Edward Diener via Boost
<[hidden email]> wrote:

> On 4/3/2017 2:45 AM, Louis Dionne via Boost wrote:
>>
>> [snip]
>
> The documentation is confusing regarding the level of C++ conformance needed
> to use the library. At the beginning it says that the library is a
> C++11/C++14/C++17 library, whatever this means. Later information explains
> that it is only dependent on a C++11 standard library. There is mention of
> compatibility claims, which further confuses the issue. Finally there is
> mention of static asserts or substitution failures for some functionality.
>
> Frankly with all this confusion I would be very loath to use the library as
> the doc explains the issue(s). It would be much clearer if the library
> expressed the minimum level of C++ compliance needed for the majority of the
> functionality and then documented any greater level of C++ conformance
> needed for specific functionality. Also the library should document a
> standard behavior when the level of conformance is not met, either internal
> adjustment to a lower level of conformance, compiler errors, static asserts,
> or run-time exceptions, depending on how the library is designed, and these
> need to be carefully explained if they differed for different functionality.
>
> The number 3) item in comparing callable traits to the current Boost
> function types needs more explanation. Having used Boost function types in
> code ( Boost tti uses it, and I have used it in other programming endeavors
> ) I do not see how the callable traits library author could think that Boost
> function types encourages template specializations, so maybe an example
> showing what is meant by item 3) would be needed to back up this claim.
> Please note: I am certainly not against modernizing the function type
> interface; I just think that claims vis a vis Boost function types need to
> be backed up with actual proof by example.
>

Thanks for the excellent feedback, Edward. I agree with your criticisms.
I will improve the documentation after the review to clarify these issues.

>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Apr 3, 2017 at 8:45 AM, Louis Dionne via Boost <
[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
On Sun, Apr 9, 2017 at 10:16 AM, Bruno Dutra via Boost
<[hidden email]> wrote:

> On Mon, Apr 3, 2017 at 8:45 AM, Louis Dionne via Boost <
> [hidden email]> 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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
On Sun, Apr 9, 2017 at 6:50 PM, Barrett Adair via Boost <
[hidden email]> wrote:
>
> On Sun, Apr 9, 2017 at 10:16 AM, Bruno Dutra via Boost
>
> > * 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.

I understand the use-case, but consider the fact that the unqualified
version should be able to work with both lvalue qualified as well as rvalue
qualified `this`, therefore there doesn't seem to be a reason to prefer the
lvalue qualifier. For the use-case you mentioned, the user could easily get
away with it by appending the desired qualifier to
qualified_parent_class_of_t<...>, i.e. & or &&, and rely on collapsing to
make it consistent.

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

That's probably a good idea.

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

On one hand I totally understand your frustration with lazy metafunctions,
but on the other hand I feel that Callable Traits should try to integrate
seamlessly with type traits provided by the standard library to ease
adoption and I think it's worth the trouble to provide them.

If you are feeling uneasy with the solution I suggested in the previous
email, you could also invert things and define the eager versions in terms
of their lazy counterparts, but that would probably require more
refactoring so as to have lazy traits somehow derive from the SFINAE
friendly implementation details, so that they also "inherit
SFINAE-friendliness" along with the presence of a nested typename type or
conversely the lack thereof.

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

Maybe "example" wasn't the best choice for a word. I was referring to a
minimal example on how to *use* the library, that is a small section that
tells the user what headers to include and, if the library supports being
used outside of the boost distribution, how to get a copy of it and set up
the project to find the correct headers. This is all trivial for a header
only library, but many users genuinely wouldn't know where to start.

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

Normally I would totally agree, but in this case I really feel it's
important that Callable Traits follow the conventions of the standard
library.

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

Somehow I missed those. Looks fine, sorry for the noise.

Bruno

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Louis Dionne
In reply to this post by Boost - Dev mailing list
Dear Boost community,

The formal review of Barrett Adair's CallableTraits library begins today,
April 3rd, and ends on April 12th.
There's just two days left for the review, and so far I count
3 formal reviews:

  Bruno Dutra
  Klemens Morgenstern
  Zach Laine

There's been other useful feedback too which I will take into account,
but not in the form of formal reviews with a clear yes/no vote. I'd like
to encourage more people to submit a formal review, as that will make
my decision much easier to make and justify.

Thanks,
Louis
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
Callable Traits solves an important problem. FWIW my Synapse library (
http://zajo.github.io/boost-synapse/) is an example of having to deal
"manually" with the problem callable traits solves, and I do plan to make
the conversion to using callable traits if it is accepted.

The documentation seems complete. I did not study the implementation, but
the unit tests are exhaustive and precise, very solid.

I think Callable Traits should be accepted.

Emil

On Mon, Apr 10, 2017 at 10:47 AM, Louis Dionne via Boost <
[hidden email]> wrote:

>
> > Dear Boost community,
> >
> > The formal review of Barrett Adair's CallableTraits library begins today,
> > April 3rd, and ends on April 12th.
>
> There's just two days left for the review, and so far I count
> 3 formal reviews:
>
>   Bruno Dutra
>   Klemens Morgenstern
>   Zach Laine
>
> There's been other useful feedback too which I will take into account,
> but not in the form of formal reviews with a clear yes/no vote. I'd like
> to encourage more people to submit a formal review, as that will make
> my decision much easier to make and justify.
>
> Thanks,
> Louis

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
I have only time to do a very brief evaluation.

I have tried this out with Clang 3.8

There are a few things which run with C++14 and not C++11 in the example code in the documentation:

    static_assert(ct::has_void_return_v<foo>, "");

    static_assert(!ct::has_varargs_v<foo>, "");

    static_assert(ct::is_const_member_v<pmf>, "");

I can understand the intention from the documentation.

I support adding this to Boost.

Best wishes

John

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CallableTraits] The formal review begins today

Boost - Dev mailing list
On Wed, Apr 12, 2017 at 7:08 AM, Fletcher, John P via Boost
<[hidden email]> wrote:

> I have only time to do a very brief evaluation.
>
> I have tried this out with Clang 3.8
>
> There are a few things which run with C++14 and not C++11 in the example code in the documentation:
>
>     static_assert(ct::has_void_return_v<foo>, "");
>
>     static_assert(!ct::has_varargs_v<foo>, "");
>
>     static_assert(ct::is_const_member_v<pmf>, "");
>
> I can understand the intention from the documentation.
>
> I support adding this to Boost.
>
> Best wishes
>
> John

Thanks, John. I should remove the variable templates from
the examples so that C++11 users don't experience confusion.

>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
12
Loading...