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-03 12:33:17

On Mon, Apr 3, 2017 at 6:06 AM, Tim Song via Boost
<boost_at_[hidden]> wrote:
> 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:
>
> - The documentation misuses the term "implementation-defined"
> throughout; "see below" seems to be closer to what is intended.
>

Agreed... "see below" would be better.

> - Technically, the fallback definitions of foo_v are all ill-formed NDR.

True, but in practice does this actually matter? I decided that this
behavior would be more user-friendly than simply removing foo_v
entirely. Is `std::is_same<T, some_hidden_type>::value`
preferred for cases like this?

> - The description of behavior seems a bit awkward due to the use of
> the passive tense: "std::false_type is inherited by
> is_lvalue_reference_member<T> and is aliased by typename
> is_lvalue_reference_member<T>::type, except when one of the following
> criteria is met, in which case std::true_type would be similarly
> inherited and aliased". Examining the implementation, it is unclear
> why the repeated alias declaration is necessary when false/true_type
> already define a member typedef 'type' that is itself.
>

Agreed. This should definitely be reworded throughout.

> - The traits do not, but should, handle top-level cv-qualification. I
> see no reason why, say, is_volatile_member<int (foo::*)() volatile> is
> true but not is_volatile_member<int (foo::* const)() volatile>.

I originally handled top-level CV. I thought that ignoring top-level CV
made the library "leaner" and easier to document, but I didn't feel
strongly about that decision. This would be easy to change.

> - "reference collapsing" behavior on the add ref-qualifier traits is
> subtle and arbitrary and I'm not aware of use cases calling for such
> rules. The FAQ's argument - that a different set of rules would be
> hard to memorize - does not sound very convincing to me. It sounds
> like the problem came from the name of the trait: "adding" a &&
> qualifier to a &-qualified function type doesn't make much sense. If,
> say, the name is recast as "make the function type && qualified", then
> the behavior would be obvious.

Fair point... I had to choose a scheme, so I went with reference
collapsing. I did try to design the library to parallel <type_traits> for the
sake of familiarity, but maybe reference collapsing goes too far.
Your suggestion makes sense to me.

> - Speaking of names, I'm not a big fan of names like
> "is_reference_member" for a trait that's actually "has ref-qualifier".
> Likewise for something like "is_volatile_member"; my first reaction is
> "is it a pointer to member data of volatile-qualified type?". Maybe
> the namespace helps, I don't know.

I really spent a lot of time thinking about names, and I was never
truly satisfied. I do like this suggestion.

> - Consider making the variable templates inline for implementations
> that support it, to match C++17.

Good catch.

> - apply_member_pointer seems to be trying to do too much. I can get a
> pointer to member data of any type, unless that data member is itself
> a pointer to member or a pointer to function, in which case I get
> something else entirely. Additionally, the description doesn't mention
> that PMDs also get special handling.

Fair point. Maybe this feature should be removed.

> - transaction_safe is not a "C++17 feature".

Yes, this was a documentation mistake that needs to be fixed.

> - Is there an explanation for qualified_parent_class_of's design,
> especially with respect to PMDs (the choice of const &)?

I found it useful, so I threw it in the library. This feature is easier
to use than combining all the `is_foo` + `add_foo` features together
with `std::conditional`. I went back and forth on the PMD case.
I ended up with `const` because it worked best for my use case.

Thanks for the great feedback, Tim.