Hi Doug, thank you for your review. Doug Gregor wrote:
I have a few comments, in no particular order:
- This library should be merged into the type traits library, both the code (headers go in boost/type_traits) and documentation (there are already some function-type facilities there that should probably be replaced). The facilities in this library are type traits, and the library itself is small enough that it does not need to stand alone.
- I'm not a huge fan of the tag types facilities. Part of the problem with them is that there are so many new names (11), many of which are very close to existing type traits: there's a tag const_qualified, for instance, which basically means is_const on the underlying function type.
That's only half of the story: The same tag means "const qualifiy" in the context of type synthesis.
At the very least, these tags will need to go in a sub-namespace of "boost" ("boost::type_traits" is fine).
We'll probably need to slightly change some names if pulling everything (except the tags) up to boost...
At best, a crazy idea: is there a way to re-use existing type traits like "is_const" instead of introducing these new names? Or, perhaps, the "non_" versions could be mpl::not_, and "tag" could be "mpl::and_". Interface reuse is really important!
That would require a small-scale reimplementation of mpl::lambda and I'm not sure it's really worth it: All tag parameters have defaults and they're probably not used that often. The names don't hurt noone when they're in their own namespace. Further, I'm not really sure it's practical: Users might assume the additional parameter works with real MPL lambda expressions (which can't, because e.g. is_const<Function>::value will never be true) and the code will read rather confusing for type synthesis, not to mention the negative impact on compile time performance. OTOH it sure sounds crazy enough to be a fun thing to implement ;-).
- I'm rather concerned about the portability of this library. Andy Little has reported some failures on MSVC 7.1,
All tests pass for MSVC 7.1. Two examples are beyond the capabilities of that compiler.
and I'm seeing a huge number of failures on Apple GCC 4.0.1.
Interesting! What does it say?
Function types will be a core part of the type traits library: they absolutely must be portable.
I agree.
- If the library is accepted, I hope the author will help us clean up other facilities in Boost
(say, result_of <g>)
Since one example implements result_of, it might be as easy as copying a file and changing the namespace in this special case.
to use this library rather than their own hacks. Part of introducing infrastructure into Boost is helping to make sure that infrastructure actually replaces what it's meant to replace.
Sure.
At this point, I don't know whether to accept or reject. The portability problem can be solved, of course, and it will be solved with time.
The issue of tag types really bothers me, because the interface is so large and seems so redundant with what we already have in type traits.
We can think of them as a "compile time enums"; would you call an enum with 10 enumerators plus one operation a large interface? The only redundancy I can see are the tags for cv-qualification.
If tag types could be simplified, my vote would be an enthusiastic "accept." Right now... I'm on the fence.
We can probably reduce the cv tags to qualified 'cv' and 'cv_not': "cv const" or "cv_not volatile" (maybe we need an optional pointer on top to work around BCC's const bug, though). Regards, Tobias