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: Antony Polukhin (antoshkka_at_[hidden])
Date: 2013-11-16 10:46:25

2013/11/16 Andrey Semashev <andrey.semashev_at_[hidden]>
<...>

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