Subject: Re: [boost] [TypeIndex] Peer review period for library acceptance begins, ending Thurs 21st Nov
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2013-11-16 07:53:18
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
> 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
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 , 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  should be taken into
account, including if these functions turn out to be implemented in
* 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
* 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();
template< typename T > type_info const& type_id(T*);
template< typename T > type_info const& type_id(T&);
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
* 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_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
And the corrections in replies to that post.
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.