Re: [boost] [Review] Boost.Type Traits Extension by Frederic Bron

Subject: Re: [boost] [Review] Boost.Type Traits Extension by Frederic Bron
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2011-03-18 17:17:40

On 3/14/2011 12:48 AM, Joel Falcou wrote:
> Dear All,
>
> This is the first day of the fast track review of Frédéric Bron's
> extensions to the Type Traits Library.This reviews will last until March
> 18th, 2011 under my management. All comments and reviews are
> very welcome.
[...]

I vote for inclusion, pending resolution of my comments below (that is, I'm
hoping the community will reach some consensus, not necessarily that all my
suggestions will be adopted).

I've perused a few pages of the documentation and a good chunk of the
implementation source.

As far as the naming, I'm fine with the has_operator_xxx convention.  In my
own implementation of such traits, I use names such as is_pre_incrementable,
is_left_shiftable, etc., which I find equally good, but as John Maddock
pointed out, the has_operator_xxx names are conveniently grouped in the
alphabetical reference.  Perhaps an additional section of the documentation
under "Type Traits by Category" should be added called "Operator Traits"
which can section out these new operator traits, and then the fact that all
the operator traits are nicely grouped together in the alphabetical
reference would be less of an issue.

For the arithmetic operators, I prefer "plus" and "minus" to "add" and
"subtract", respectively, and likewise "plus_equal" to "add_assign", but
it's not a strong preference, and perhaps consistency with other Boost
libraries should dictate these, I don't know.  I do more strongly prefer
"post_decrement" to "postfix_decrement", and to have "increment" and
"decrement" spelled out.  "bit_op" versus "bitwise_op" is another one that I
have no strong preference either way, so I would probably look at other
Boost libraries for precedent.

One could provide aliases for the more contentious trait names...?  There is
precedent for this in Boost.Fusion with accumulate and fold...

At least one example in the documentation utilizing one of these operator
traits would be useful.  I like the idea posited earlier of a generic
streaming function, maybe for debugging purposes, which uses << if
available, else streams out some default string containing the type of the
argument.

It is implicit in the documentation, and likewise verified from looking at
the implementation, that all arguments are assumed to be lvalues.  That is,
a reference qualifier is automatically added to the argument types given to
an operator trait.  Thus, the only way to query if an operator is applicable
to an rvalue is to query using a const-qualified type, and even then I think
you could get back false negatives in C++0x and even C++03 with Boost.Move
rvalue-reference emulation (consider only operator_xxx(T&&) available).
Further, rvalueness and lvalueness does enter into overload resolution
(rvalues can't bind to T& when T is deduced), hence it does influence the
result type of the operation.  For these reasons, I believe the traits
should strive to differentiate between lvalue and rvalue argument types.

In either case, whether arguments are treated as presently or as
propositioned above, this treatment of the arguments should be spelled out
explicitly.

As Vicente pointed out, the use of make in the implementation can be
replaced with the declval that common_type uses.

I don't have strong feelings on whether to use "void" to signify that one
"doesn't care" about the result type or to use some "dont_care" type.  I've
personally used "void" in my operator traits implementation.  If the latter
solution is settled upon, I believe it should be a documented part of the
interface, and not in a detail namespace, to allow users to explicitly
supply it.

I still believe an extra template parameter representing a Boost.MPL
metafunction to apply to the result type would be useful, relatively easy to
add, and will not change the existing interface.  It seems, however, that
I'm the only one that likes this idea, and it's use cases are relatively
limited.  I've mostly used such a feature to further constrain the result
beyond just "convertible".  For example, one may want to prevent returning
references to locals from functions.  That is, the function's return type is
fixed to be T const &, and the function logically returns the result of an
operator applied to a local variable.  If the operator returns U which is
implicitly convertible to T, then everything compiles fine but it's probably
asking for a crash, as the function's return value is a dangling reference.
You'd hope you'd get a compiler warning, but better to either create a hard
error in such cases or provide an alternate implementation of the function.

I think that covers everything.  Well done Frederic!

- Jeff