2013/11/16 Andrey Semashev
2. Any conditions which should be attached to acceptance into Boost e.g. fixes, additional testing, changes to documentation. Please be as specific as possible here (bullet points are good!)
While reviewing the library I inspected the code and the documentation (mostly the Getting started and Examples sections). I did not compile the code or tests.
There are a few critical issues that need to be resolved before the inclusion:
* The undefined behavior of casting std::type_info const& to boost::type_info const& has to be removed. I described my preferred solution earlier in the discussion [1], but that particular solution is not a requirement. Anthony can of course choose another solution, as long as it solves the problem and does not undermine the library utility.
Question that disturbs me is, what type must be returned by the type_id<T>() method. Returning just a const std::type_info& seems almost useless, returning const boost::type_index& is not very portable/thread-safe because requires static const type_index variable construction. Must it return a type_index?
* BOOST_CLASSINFO_COMPARE_BY_NAMES macro should be named with the library name as its part (e.g. BOOST_TYPE_INDEX_COMPARE_BY_NAMES). It should probably be made user-definable, if this workaround is needed on some platform Boost doesn't cover. Also, I'm not sure it should be defined by default on gcc <4.5 and Intel on Linux. I'm pretty sure gcc on Linux worked fine since 4.1, if symbol visibility is set correctly (some later version of gcc added an attribute to simplify this) and it's hard to believe that no version of Intel works. Are these checks confirmed by tests?
It is a known bug with GCC on ARM architecture. Additional testing required to check, is it safe to use GCC's comparisons on x86. This can be done later, not during the library review.
* I don't think type_id_rtti_only() is a good name. I'd suggest making these functions overloads of type_id() with no arguments and simply make them optional with #ifndef BOOST_NO_RTTI. I.e.:
template< typename T > type_info const& type_id();
#ifndef BOOST_NO_RTTI template< typename T > type_info const& type_id(T*); template< typename T > type_info const& type_id(T&); #endif
Returning just a std::type_info will require additional care from the user to compare type_infos correctly. Returning a type_index will solve that problem. How about just having a copyable, hashable boost::type_index class that is comparable with std::type_info (something close to v1.0 of TypeIndex)? In that case the library will have anly a boost::type_info typedef, boost::type_index class for RTTI and boost::template_index for situation when RTTI is off. -- Best regards, Antony Polukhin