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-14 02:48:08

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

> On Thu, Nov 14, 2013 at 9:26 AM, Antony Polukhin <antoshkka_at_[hidden]>
> wrote:
> > 2013/11/14 Steven Watanabe <watanabesj_at_[hidden]>
> >
> >> AMDG
> >>
> >> On 11/12/2013 11:34 AM, Niall Douglas wrote:
> >> > Boost community feedback is requested for the formal peer review of
> >> > the TypeIndex library by Antony Polukhin. Feedback requested
> >> > includes:
> >> >
> >> > 1. Should this library be accepted into Boost?
> >> >
> >>
> >> No.  This library should not be accepted into Boost
> >> in its current form.
> >>
> >> type_info.hpp:139:
> >>         return static_cast<const boost::type_info&>(typeid(type));
> >>
> >> This is undefined behavior, and there is no
> >> way to make it correct.
> >>
> >
> > This is a necessary evil and it is harmless:
> > * all the methods of std::type_info (except destructor) are non virtual,
> so
> > for example calls to name() will call boost::type_info::name()
> > * it is *undefined* which destructor (std::type_info or boost::type_info)
> > will be called *but*:
> >   * boost::type_info contains no members, it's size must be equal to the
> > size of std::type_info, so no harm if boost::type_info destructor won't
> be
> > called
> >   * destructor of std::type_info will be called anyway
> >
> > I can add assert to tests to ensure that sizeof(std::type_info) ==
> > sizeof(boost::type_info).
>
> I think the undefined behavior can be removed with the following trick:
>
> 1. boost::type_info should not derive from std::type_info (it would
> have no base classes or data members). Its destructor should be
> deleted, as well as its constructors and assignment.
> 2. The above cast should be replaced with reinterpret_cast:
>
>   return reinterpret_cast<const boost::type_info&>(typeid(type));
>
> 3. All methods of boost::type_info should perform the reverse
> reinterpret_cast of *this before accessing std::type_info.
>
> The standard warrants that such two-way casting would yield the
> original reference to std::type_info.
>
> However, this turns out to be quite a lot of hackery and makes me
> wonder if boost::type_info is needed at all. Why not just have
> boost::type_index that will work directly with std::type_info?

Unfortunately, boost::type_info must derive from std::type_info for
compatibility reasons. Without it the following code won't compile:

std::type_index ti = boost:type_id<int>();

This may look like a rare case, however will be quite common if other Boost
libraries adopt the TypeIndex. Is my favorite example with Boost.Any:

boost::any a = 10;

// Imagine that Boost.Any uses boost:;type_info instead of std::type_info
class_that_constructs_from_std_type_info = a.type();

// operator const std::type_info&() won't help
std::type_index ti = a.type();

In v1.0 of TypeIndex library there was no boost::type_info, there was only
boost::type_index. version 2 was started because v1.0 was hard to use in
existing Boost libraries that that return std::type_info to the user.

Version 1.0 is not a drop-in replacement. It can be used only if we have
full control over the code that uses it.

Why there is a such need in drop-in replacement? Here are the reasons:

* some compilers fail to compare std::type_info across modules (surprise!)
* some libraries fail to write a sane hashing function for std::type_info
* users want to use Boost without RTTI and fail:
http://stackoverflow.com/questions/14557313/is-it-possible-to-use-boost-program-options-without-rtti

Here are some more issues in Boost:
* there is a mess with stripping/not stripping const, volatile, reference
* there is a mess with name() calls, plenty of places where name() used for
debug/user output without demangling
* some of the Boost libraries demangle names by themselfs and forget to
dealocate memory (at least valgrind says so)

In three words: std::type_info is broken.

There is a need to give users an ability to write portable code, but do not
break existing users code (that do fail on some compilers). There is a need
to put all the std::type_info workarounds all around the Boost in s single
place, which will be easy to maintain. It is not good to see in different
libraries code like that:

// Borrowed from Boost.Python library: determines the cases where we
// need to use std::type_info::name to compare instead of operator==.
#if defined( BOOST_NO_TYPEID )
#  define BOOST_FUNCTION_COMPARE_TYPE_ID(X,Y) ((X)==(Y))
#elif (defined(__GNUC__) && __GNUC__ >= 3) \
 || defined(_AIX) \
 || (   defined(__sgi) && defined(__host_mips))
#  include <cstring>
#  define BOOST_FUNCTION_COMPARE_TYPE_ID(X,Y) \
     (std::strcmp((X).name(),(Y).name()) == 0)
# else
#  define BOOST_FUNCTION_COMPARE_TYPE_ID(X,Y) ((X)==(Y))
#endif

Or like this:

// See boost/python/type_id.hpp
// TODO: add BOOST_TYPEID_COMPARE_BY_NAME to config.hpp
# if (defined(__GNUC__) && __GNUC__ >= 3) \
 || defined(_AIX) \
 || (   defined(__sgi) && defined(__host_mips)) \
 || (defined(__hpux) && defined(__HP_aCC)) \
 || (defined(linux) && defined(__INTEL_COMPILER) && defined(__ICC))
#  define BOOST_AUX_ANY_TYPE_ID_NAME
#include <cstring>
# endif

Did you note that macro *differ*? And by the way, in both cases it is not
optimal!

-- 
Best regards,
Antony Polukhin