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 19:34:06

On Sun, Apr 9, 2017 at 6:50 PM, Barrett Adair via Boost <
boost_at_[hidden]> 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