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 11:52:40

On Saturday 16 November 2013 19:46:25 Antony Polukhin wrote:
> 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?

My understanding was that type_id() is intended to be the portable alternative 
for operator typeid(). If that's the case it should return boost::type_info 
const&.

However, I can see the convenience of returning type_index right away. This 
will encourage the use of type_index instead of type_info and maybe that's the 
right thing to do.

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

Then the condition should also include checks for the ARM target (see 
boost/atomic/detail/platform.hpp for example).

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

Yes. Ok, let it return type_index then.

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

I don't think there's any use for template_index, as I outlined in my review. 
type_index already has all the functionality and should work whether or not 
RTTI is enabled.

type_index need not be directly comparable with std::type_info because it is 
implicitly convertible from it. The comparison operators between just 
type_indexes will work.

Other than that such design looks good to me.