On Mon, Apr 3, 2017 at 6:06 AM, Tim Song via Boost
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
- 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
is true but not is_volatile_member .
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
- 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.