[boost] Peer Review Report for proposed Boost.TypeIndex v2.1 Nov 12th – 21st 2013

Subject: [boost] Peer Review Report for proposed Boost.TypeIndex v2.1 Nov 12th – 21st 2013
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2013-11-24 17:29:52

Please let me know as soon as possible any errata, missing credits etc.

Niall

 

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/.

Source: https://github.com/apolukhin/type_index/zipball/master

Documentation: http://apolukhin.github.com/type_index/index.html

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.

Contents:

·         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.

 

[I agree]

 

·         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:

Text Box: /*! Returns a static const char string of unknown format uniquelyidentifying this type.The only guarantee is that this string will beunique to the type within this process lifetime. */const char *unique_name() const noexcept;/*! Returns a representation of this type suitable for printing.This call may take some time as its storage may not be cached. */std::string pretty_name() const;class enum mangling{Native, //!< Whatever the native mangling used by this toolset isMSVC, //!< The Microsoft C++ mangling formatItanium //!< The Itanium C++ mangling format};/*! Returns the mangled form of the string representation of the type.After the calculation the value is cached statically such that the c_str()function can be used to convert the returned string to a const char *format identical to what may be returned by std::type_info::name() (orraw_name() on MSVC).This function may throw an exception if it does notsupport mangled type string calculation, including when mangling=Native onsome 

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.

Niall Douglas
Waterloo, Canada, November 2013