Re: [boost] Peer Review Report for proposed Boost.TypeIndex v2.1 Nov 12th – 21st 2013

Subject: Re: [boost] Peer Review Report for proposed Boost.TypeIndex v2.1 Nov 12th – 21st 2013
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2013-11-24 22:01:44

On Sunday 24 November 2013 17:29:52 Niall Douglas wrote:
> 
> ·         It was mentioned that some function-local static data members are
> initialised in a thread unsafe way.
> 
> [For information I have to hand a very lightweight
> BOOST_BEGIN_MEMORY_TRANSACTION() implementation ideal for this purpose. We
> could submit it to Boost.Detail and then everyone could use it?]

There is call_once in Boost.Thread and there will be once blocks and call_once 
in Boost.Sync. Do we need another reimplementation?

> My recommendation to the library author and the Review Wizards:
> 
> 
> ·         In my opinion Antony ought to make TypeIndex v3 quite literally a
> very lightweight container of some unknown, but known to be uniquely
> identifying for some type, static const char * string. I think its class
> type and its list of member functions ought to be deliberately compile-time
> incompatible with std::type_info to force authors to upgrade their code. A
> conversion member function ought to be able to synthesise a corresponding
> std::type_info using typeid() from some boost::type_index<T>, but that’s
> about it. I would even, personally speaking, go so far as to only provide a
> boost::type_index and no corresponding boost::type_info, especially if the
> boost::type_id<T>() function can return a const boost::type_index<T>& and
> therefore can be used as a static const lref, or copy constructed from it
> etc.

That's an unexpected turn. If boost::type_id<T>() returns a reference to a 
non-copyable object then it is useless for me because the essential advantage 
of type_index is its value semantics. And in such a design boost::type_index 
has very different semantics from std::type_index, so it shouldn't be named as 
such to avoid confusion. I would vote against accepting such design.

If you are willing to follow that design then I'd like to ask you to rename 
the library and the type_index type to avoid the confusion and move all types 
and functions into the specific library namespace.

> A suggested name() member function replacement which correctly breaks out
> the multiple confounding uses of std::type_info::name() into each of their
> three use cases (and which intentionally causes any use of name() to fail
> to compile) might be:
> 
> Text Box: /*! Returns a static const char string of unknown format
> uniquelyidentifying this type.The only guarantee is that this string will
> beunique to the type within this process lifetime. */const char
> *unique_name() const noexcept;/*! Returns a representation of this type
> suitable for printing.This call may take some time as its storage may not
> be cached. */std::string pretty_name() const;class enum mangling{Native,
> //!< Whatever the native mangling used by this toolset isMSVC, //!< The
> Microsoft C++ mangling formatItanium //!< The Itanium C++ mangling
> format};/*! Returns the mangled form of the string representation of the
> type.After the calculation the value is cached statically such that the
> c_str()function can be used to convert the returned string to a const char
> *format identical to what may be returned by std::type_info::name()
> (orraw_name() on MSVC).This function may throw an exception if it does
> notsupport mangled type string calculation, including when mangling=Native
> onsome
> 
> Bear in mind that user code can always subclass boost::type_index and add
> their own name() implementation based on one or more of the above new
> member functions.

I don't think that mangled_name() (the last function that returns the cached 
mangled name) is a good idea. Requiring reimplementation of some particular 
mangling schemes (and by the way, why these specific ones?) is an unnecessary 
burden on the author (and maintainer later). Caching the name internally as a 
static member complicates the design (the cache has to be thread-safe and be 
accessible in global constructors/destructors). Caching the name as a regular 
member is unacceptable bloat (people expect type_index to be as light as a 
pointer to type_info; no dynamic memory allocation and associated possible 
exceptions are allowed).

I stand by the three functions we discussed during the review:

// Returns std::type_info::name()
const char* name() const noexcept;
// Returns some low-level (possibly mangled) name
const char* raw_name() const noexcept;
// Returns some possibly human readable name
std::string pretty_name() const;

I'll add that I still think that the first one is required (at least, for 
compatibility with std::type_index) and has ho have exactly that semantics.