On Tuesday 12 November 2013 14:34:27 Niall Douglas wrote:
Boost community feedback is requested for the formal peer review of the TypeIndex library by Antony Polukhin.
First, let me thank Anthony for bringing this library to Boost. I think this is a long overdue addition, we've reinvented that wheel too many times already.
Feedback requested includes:
1. Should this library be accepted into Boost?
Yes, if the critical conditions below are met (otherwise no). After the required modifications to the library are made, another review is needed, maybe a fast track one.
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. * template_info construction involves unprotected function-local static variables and therefore is not thread-safe. The library should be thread-safe, even in C++03. * template_info::construct* functions should not be part of the class interface (or implementation). These functions should be an implementation detail of template_id() functions. The same is true for type_info::construct*, if type_info is preserved as a class after resolving the UB. In the end I'd like boost::type_info and boost::template_info interface and semantics to be as close to std::type_info as possible. * cvr_saver<> should be marked with BOOST_SYMBOL_VISIBLE. * The result of the discussion about name functions [2] should be taken into account, including if these functions turn out to be implemented in 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? * Continuing the previous point, I can see that user_defined_namespace::user_defined class in libs/type_index/test/test_lib.cpp and testing_crossmodule.cpp are not marked with BOOST_SYMBOL_VISIBLE. This can make the cross-module test fail on a platform that actually supports cross-module RTTI comparison. The class should be marked visible for this test to work correctly. It may be worthwhile to move the class definition to test_lib.hpp to avoid duplication. * I'm not sure what is the purpose of the template_index typedef. It's not covered in the docs and it offers no advantage over type_index. I think, it is better to reduce responsibilities of the current template_info class so that it only mirrors std::type_info (i.e. it is nothing more than a low level type descriptor). Maybe even it should be only available as an implementation detail when RTTI is disabled. All the additional functionality, like ordering operators and name functions, are already provided by type_index, which should be used portably with and without RTTI enabled. In that case template_index is redundant and should be removed. * For the above point, tempate_info should not be copyable or assignable. It should not have stream operators as well. * I don't think that testing for macros like __FUNCSIG__ or __PRETTY_FUNCTION__ outside of a function body is correct. These are not the real macros, in the way they are only defined in a function body. There is also the standard __func__ macro that could be used by the library. I think that all the macro checks in template_info.hpp should be moved to the ctti<T>::n() function body. There is also no need for the lazy_function_signature_assert() function in that case (the assert can be embedded into n() directly). There are also some non-critical issues and wishes that I would like to be solved by the library. These points do not preclude the inclusion of the library and may be topics of discussion. * I'd like specialized functions template_id_with_cvr() and type_id_with_cvr() along with cvr_saver<> to be extracted to separate headers. The rationale for this is that this is an optional feature that is not always required. Separating it to dedicated headers can make the main headers more lightweight. * It might be a good idea to add template_info and type_info generator functions that do not preserve CVR qualification but still wrap the type with cvr_saver. The point of this is that cvr_saver would guarantee that the type is marked visible while the original type may not be. I'm using this trick in Boost.Log [3]. * 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 I think it is obvious enough that these functions obtain the dynamic type of the referred object and thus require native RTTI. * The support for std::hash can be added in a separate support header, that would be included by the user, if he wants to. By default, no other library headers would include this glue header, so there will be no compatibility issue if the particular standard library does not provide std::hash. The support for boost::hash (through the hash_value function) can be left as is. * In type_info::before() and hash_code() you unnecessarily call std::type_info::name() multiple times. This would likely be a library call. Please, avoid it, you can cache the name() result and use it multiple times. * The documentation could use a section explicitly describing the differences between the library behavior with and without native RTTI. Perhaps, a table comparing the two modes would be useful. * On platforms where we know we will be comparing type names when comparing std::type_infos (this includes Windows since MSVC does it internally, AFAIK) we could try first comparing the address of the type_infos, and only if it differs pass on to type_info::operator==. This may provide some speedup if the type_infos reside in a single module. * In template_info.hpp, please, don't include is_arithmetic.hpp, unless it's actually needed. * BOOST_TYPE_INDEX_FUNCTION_SIGNATURE macro could be made user-definable in case if there is some way to generate name strings not supported by Boost. * All configuration macros that affect the library (e.g. BOOST_TYPE_INDEX_FORCE_NO_RTTI_COMPATIBILITY, BOOST_TYPE_INDEX_COMPARE_BY_NAMES and BOOST_TYPE_INDEX_FUNCTION_SIGNATURE) should be documented in a separate top-level section. BTW, may I suggest renaming BOOST_TYPE_INDEX_FORCE_NO_RTTI_COMPATIBILITY to just BOOST_TYPE_INDEX_FORCE_NO_RTTI. [1] http://article.gmane.org/gmane.comp.lib.boost.devel/246314 And the corrections in replies to that post. [2] http://article.gmane.org/gmane.comp.lib.boost.devel/246256 [3] http://svn.boost.org/svn/boost/trunk/boost/log/detail/visible_type.hpp PS: Wow, this turned out to be a much longer list than I anticipated. But let that not scare you, Anthony, overall the library is good and very useful, and I can't wait to see its inclusion.