Re: [boost] [TypeIndex] Peer review period for library acceptance begins, ending Thurs 21st Nov

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