Please let me know as soon as possible any errata, missing credits etc.
Boost Peer Review Report for proposed Boost.TypeIndex v2.1 Nov 12th – 21st 2013
This report is the first edition, dated Sunday 24th November 2013. The review manager was Niall Douglas http://www.nedprod.com/.
Peer review discussion detail: http://boost.2283326.n4.nabble.com/TypeIndex-Peer-review-period-for-library-acceptance-begins-ending-Thurs-21st-Nov-tt4654788.html
My thanks to the Boost members whose peer review makes up this report: Robert Ramey, Vicente J. Botet Escriba, Gavin Lambert, Andrey Semashev, Steven Watanabe, pfultz2, Mathieu Champlon, Joël Lamotte Klaim, Rob Stewart, Jan Herrmann, Tony Van Eerd.
· Summary of consensus from peer review ......................................................................................................... 1
· My recommendation to the library author and the Review Wizards ................................................................ 3
Summary of consensus from peer review:
The consensus feedback was to accept TypeIndex after substantial modifications. There were two recommendations of rejection due to how substantial such modifications would need to be (i.e. a request for a second round of peer review with a new implementation).
Note that my personal opinion based on post-review reflection is written in [square bracketed italics].
· Many seemed to feel that a boost::type_info’s member function API, where identically named to that of std::type_info member function API, ought to have identical compile-time effects. This implied that missing member function APIs, or other compile time errors for missing or incomplete functionality, would be okay but if name() returns some const char *, then any const char * so long as it meets the C++ standard is okay.
[I think there are three use cases for a type_info like substitute: (i) as a lightest possible weight, non-RTTI requiring minimal type_info which can have any API it likes and ought to not be directly substitutable for std::type_info or std::type_index (ii) as a compile-time substitute for std::type_info in that it will compile without third party code modification, but may not produce reliable code due to not returning exactly the same values as std::type_info (iii) as a runtime substitute for std::type_info in that it will require code to be “ported” to it due to having a breaking API, but thereafter produces reliable code. I’ll be very blunt in saying that (ii) is a bad idea in my opinion, and (iii) is valuable but out of scope enhancement which could be added as a separate class after peer review. In my opinion, (i) is where we ought to focus, and I would recommend that the lightest possible weight type_info replacement be deliberately compile-time incompatible with std::type_info (where output is not identical to std::type_info) to force porting code to it so authors are not lazy.]
· A large minority seemed to feel that a boost::type_info’s member function API, where identically named to that of std::type_info member function API, ought to have identical run-time outcomes. Most specifically, this would mean that member functions e.g. name() would return exactly what std::type_info’s name() does, even if for example name() returns a std::string instead of a const char * as would be necessary in pre-C++11 compilers.
[This is really use case (iii) in the previous item.]
· It was mentioned that it should be possible to optionally specialise std::hash<> for type_info for optional seamless use in hash taking containers.
· Some felt that implementation-specific std::type_info quirks ought to be replicated. Others felt they should not, and a portable interface which works identically on all platforms is preferred.
[This is really use case (iii) in the previous item.]
· boost::type_info is currently initialised via a static cast from a std::type_info when RTTI is available. This is undefined behaviour, and some felt this to be a showstopper. I (Niall Douglas) looked into this more deeply due to its seriousness, and found that because most STL implementations define std::type_info with a virtual function table, the undefined behaviour static cast would cause dynamic_cast on a boost::type_info instance to misoperate.
[I believe the RTTI induced misoperation to indeed be a showstopper, and that this use of undefined behaviour to reduce code bloat is an unsafe optimisation. A much lighter weight non-direct-substitute for std::type_info ought to be as code bloat parsimonious as is possible, while its lack of non-explicit interoperation ability with std::type_info ought to allow avoidance of needing any UB tricks.]
· Some felt that any use of implied boost::type_info ought to be explicitly written as boost::type_info instead of relying on namespace lookup, as the name similarity to std::type_info may introduce confusion.
· It was mentioned that some function-local static data members are initialised in a thread unsafe way.
[For information I have to hand a very lightweight BOOST_BEGIN_MEMORY_TRANSACTION() implementation ideal for this purpose. We could submit it to Boost.Detail and then everyone could use it?]
· Magic macros such as __PRETTY_FUNCTION__ ought to not be referenced outside of a function as they don’t technically exist there. Checks for their existence ought to be within a function.
· It was requested that a boost::type_id<> which takes some unknown variable instance as a parameter and therefore allows the compiler to deduce the type of the variable as the boost::type_index<T> type would be valuable.
· A mechanism for programming compile-time logic with class inheritance trees such that inheritance can be determined at compile-time with RTTI disabled was requested.
My recommendation to the library author and the Review Wizards:
· In my opinion Antony ought to make TypeIndex v3 quite literally a very lightweight container of some unknown, but known to be uniquely identifying for some type, static const char * string. I think its class type and its list of member functions ought to be deliberately compile-time incompatible with std::type_info to force authors to upgrade their code. A conversion member function ought to be able to synthesise a corresponding std::type_info using typeid() from some boost::type_index<T>, but that’s about it. I would even, personally speaking, go so far as to only provide a boost::type_index and no corresponding boost::type_info, especially if the boost::type_id<T>() function can return a const boost::type_index<T>& and therefore can be used as a static const lref, or copy constructed from it etc.
A suggested name() member function replacement which correctly breaks out the multiple confounding uses of std::type_info::name() into each of their three use cases (and which intentionally causes any use of name() to fail to compile) might be:
Bear in mind that user code can always subclass boost::type_index and add their own name() implementation based on one or more of the above new member functions.
· In other words, here I think “less is more” in every way you look at it. I think a slimmer less heavy TypeIndex would be a superior solution.
· I request the Review Wizards to allow Antony some time to make the changes to produce a v3 of the library, and then to allow TypeIndex v3 a Fast Track Review of five days to ensure the Boost community is happy and that its feedback has been incorporated. It was asked during peer review that such a five day period please include a weekend. I would be happy to serve as review manager again if requested.
Waterloo, Canada, November 2013