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

Subject: Re: [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 18:01:53

Looks like the HTML formatting got mangled by the list bot gods, so 
the below is not particularly readable. You can find a PDF version at 
https://drive.google.com/file/d/0B5QDPUNHLpKMakQ2TXVGcHNkZ1E

Niall

On 24 Nov 2013 at 17:29, Niall Douglas wrote:

> 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 cha * 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 oeported to it due to having a breaking API, but 
thereafter produces reliable code. ITMll 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 car * 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 thatTMs 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
> 
>  
> 
> 

-- 
Currently unemployed and looking for work.
Work Portfolio: http://careers.stackoverflow.com/nialldouglas/