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