Re: [boost] [typeindex v3.0] Peer review begins Mon 21st ends Wed 30th

Subject: Re: [boost] [typeindex v3.0] Peer review begins Mon 21st ends Wed 30th
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2014-04-28 04:11:54

On Sunday 27 April 2014 22:33:09 Antony Polukhin wrote:
> 2014-04-27 18:18 GMT+04:00 Andrey Semashev <andrey.semashev_at_[hidden]>:
> 
> 1.2. I don't like the stl_type_index name. Both stl_type_index and
> 
> > ctti_type_index attempt to implement STL interface (with benefits), so
> > stl_type_index doesn't sound right to me. As a complement to
> > ctti_type_index,
> > I would suggest rtti_type_index.
> 
> This is not true for some of the compilers. See answer to 2.4
> 
> > 2.4. Why do you always use std::type_info in case of MSVC? I'm seeing
> > BOOST_MSVC in the condition in boost/type_index.hpp. Why not use
> > ctti_type_index like with every other platform?
> 
> Type info provided by MSVC is much more compact and sometimes works
> slightly faster that ctti_type_info.

I see. Still, I'd call std::type_info RTTI even if it does not implement all 
features. stl_type_index doesn't sound right to me.

> >     - pretty_name() is equivalent to name(), if not re-defined in the
> > 
> > derived
> > class. The default definition (calling the Derived::name()) should be
> > present
> > in type_index_facade.
> 
> I'd better require this function to be defined in Derived class

Why? I don't think it should be mandatory. After all, there may be no way to 
provide such a string except to fall back to name() or raw_name().

> > 1.4. Remove the undocumented protected members from type_index_facade.
> > This
> > class is intended to be derived from, so protected members should be
> > either
> > part of the interface or not present. As it is, the methods are simply
> > declared and not defined (in type_index_facade), I don't see the point of
> > these declarations.
> 
> They are only declared for Doxygen. Without those declarations there is no
> nice way to make doxygen print info for those methods on type_index_facade
> page.

Doxygen generates Reference section, it is a formal description of the class' 
interface. There are no such methods in type_index_facade, so you basically 
make the description incorrect.

What you need is a separate section in the docs describing the library 
extension by deriving from type_index_facade. In that section you can document 
what member functions are required to be defined in the derived class.

> > 1.5. I don't think BOOST_TYPE_INDEX_USER_TYPEINDEX is a good idea. This is
> > a
> > very easy way to shoot yourself in a foot with ODR issues and ABI
> > incompatibility. Other Boost libraries that might be using Boost.TypeIndex
> > (I'm having Boost.Log in mind now) should be confident which version of
> > type_index they are using. That includes both the library and the user's
> > application compilation stages. IMO, there should be stable type_index
> > types,
> > rtti_type_index and ctti_type_index, and type_index should be one of them.
> > I
> > don't really see a use case for BOOST_TYPE_INDEX_USER_TYPEINDEX macro
> > (i.e. I
> > don't see why a user would want to implement his own type_index and force
> > Boost to use it with that macro). Please, remove it or at least make it
> > affect
> > a separate set of typedefs (e.g. configured_type_index and
> > configured_type_info).
> 
> Well it's a sharp tool. You could get cut if use it without care.
> Use case is pretty simple and common in embedded development: developer do
> not like RTTI. There are cases when something-like-RTTI is required with
> -no-rtti compiler flag. In that case ctti_type_info is used by default. But
> that could not be enough for some of the users. That's the case for
> BOOST_TYPE_INDEX_USER_TYPEINDEX. If Boost user uses only 5-20 types with
> Boost libraries, then user could provide 5-20 (much more compact)
> type_infos and use Boost on embedded device without RTTI.
> 
> That's only what pops in my head. Maybe user's will came up with something
> more.

I'm ok with user wanting to implement his own RTTI backend, you provide 
type_index_facade for that purpose. What I'm not ok with is making this user-
defined implementation the default for Boost. This affects ABI and possibly 
the behavior of a core component. This would be a blocker for me to use it in 
Boost.Log.

> > 1.6. Suggest choosing a more obscure virtual function name defined by
> > BOOST_TYPE_INDEX_REGISTER_CLASS to avoid name clashes with user's symbols
> > and
> > make it less apparent in IDEs' code completion. I'd suggest something like
> > _boost_type_index_type_id_runtime().
> 
> Good point. But I'd rather not start it from underscore and change it to
> boost_type_index_type_id_runtime_()

Ok, works too.

> > 2.5. stl_type_index::pretty_name(). I know this shouldn't happen, but the
> > last
> > two loops may exceed the buffer size. Please, add (pos < size) to the
> > first
> > loop condition,
> 
> raw_name() must return zero terminated string. Additional check in a loop
> is not required here.

ret[pos] when pos == ret.size() is illegal. Even if std::string actually 
stores the terminating zero, it is not part of the character sequence 
accessible through indexing operator. So the check is needed.

> > (end != npos) check before the second loop and (end > pos) to
> > the second loop condition. You never know what string the compiler gives
> > you,
> > so be prepared.
> 
> Chances are +0 but that won't do harm here. Ok.

Again, I'll reiterate. The code is dealing with compiler-specific strings, so 
it should be prepared for any content.

> > 2.6. Actually, make stl_type_index::pretty_name() return name() in case of
> > any
> > 
> > failure, except bad_alloc. That includes the following cases:
> >     - __cxa_demangle failed and returned a non-zero status and/or a null
> > 
> > pointer
> > 
> >     - the following parsing fails to recognize the demangled string (e.g.
> > 
> > you
> > don't find the closing angle bracket or the resulting string is empty).
> 
> This looks like a bad idea. We're silently ignoring an error and changing
> function result according to it. User won't be able to determinate the
> state returned string (pretty or name()?).

You don't give any guarantees for pretty_name() result, only that it is 
possible more readable than other kinds of names. Falling back to name() looks 
like a reasonable error handling to me. An exception doesn't do any good. The 
primary use of this function is for diagnostic purposes, so the user wants to 
output something. If not pretty_name() then the next best thing - name(). 
Throwing an exception from pretty_name() only requires the user to wrap it 
with try/catch and output name() in the catch block.

Also, if your code is run on a platform with unexpected format of the 
demangled strings your pretty_name() would be always throwing. This is hardly 
a desired behavior. IMO, pretty_name() should do its best to provide a 
readable name but not fail when it can't.

> > 2.8. stl_type_index::type_id(). Why is the EDG-compilers with arithmetic
> > types
> > a hard error? I think, this compiler bug should be either concealed (e.g.
> > by
> > changing the signed int type to int) or exposed (i.e. just apply typeid)
> > but
> > not prohibit from compilation. I might never use signed ints in my
> > (user's)
> > code, so why am I not allowed to write type_id<int>()?
> 
> This code is taken from Boost.Python. I have no ancient EDG based compilers
> nearby, so I was afraid to change something. I have no idea how type traits
> would work on that compiler or would a self made type trait work.

I think it is safe to assume that the compiler supports template 
specialization (at least, full specialization). Otherwise Boost.TypeTraits you 
already use (as well as pretty much all Boost) wouldn't work. In any case, the 
current implementation is not correct.

> > 2.10. In stl_type_index::type_id_with_cvr(), you should probably use
> > mpl::or_
> > instead of boolean operators since some compilers don't support that well.
> > I
> > remember some MSVC versions had such problems, not sure if they fixed it.
> 
> Are we still supporting those? I was thinking that after 7.0 MSVC had no
> such issue.

This needs to be checked. I don't have access to MSVC right now, maybe later.

BTW, you could avoid mpl::or_ and instead use a local enum to calculate the 
condition result. This works well with all compilers to my knowledge.

> > 2.12. Again, I would suggest a safer code in
> > ctti_type_index::pretty_name()
> > and ctti_type_index::hash_code(), where it comes to
> > detail::ctti_skip_size_at_end accounting. First, adding this constant to
> > the
> > strlen argument is, well, counterintuitive; it too me several seconds to
> > realize what was actually meant here. Second, I'd suggest checking that
> > ctti_skip_size_at_end is less that strlen(raw_name()) before subtracting
> > it.
> > Third, ctti_skip_size_at_begin accounting should also be made with safety
> > in
> > mind. Ideally, I would move all this accounting to
> > ctti_type_index::pretty_name() (and made ctti::n() return the oritinal
> > string). ctti_type_index::pretty_name() should perform strlen and then
> > verify
> > that skipping ctti_skip_size_at_begin and ctti_skip_size_at_end characters
> > would be valid and result in a non-empty string. Otherwise return the
> > original
> > string as is. I know you have tested these constants with select compilers
> > and
> > you're sure they are correct. But you never know how the compilers may
> > change
> > over time and you can never support all pretenders for MSVC and GCC to
> > rely on
> > the particular string formats. So the code must work safely in all cases,
> > even
> > the most ridiculous ones.
> 
> I think we could do better even. All those assertions could be made at
> compile-time. ctti::n() works with arrays of chars and all the ctti_skip*
> are compile-time constants.

Compile-time would be great but I think it should not be assertions. Instead, 
if the constants appear to be invalid, return the whole string with no 
skipping. Again, strive for improvement but not require it.

I think you could create a POD structure like this:

  struct name_descr
  {
    // Full string as reported by the compiler
    const char* raw_name;
    std::size_t raw_size;

    // Adjustments that need to be made to get pretty name
    std::size_t pretty_offset;
    std::size_t pretty_size;
  };

You could create a function-local static object of this structure and return 
it instead of const char* from ctti<T>::n(). You only have to statically 
initialize this struct with the correct values.

  static const name_descr& n() BOOST_NOEXCEPT
  {
    static const name_descr descr =
    {
      __func__, // or whatever other macro
      sizeof(__func__) - 1,
      (ctti_skip_size_at_begin + ctti_skip_size_at_end) >= sizeof(__func__) - 
1 ? 0 : ctti_skip_size_at_begin,
      (ctti_skip_size_at_begin + ctti_skip_size_at_end) >= sizeof(__func__) - 
1 ? sizeof(__func__) - 1 : sizeof(__func__) - 1 - ctti_skip_size_at_end
    };

    return descr;
  }

Strictly speaking, in C++03 this initialization is still allowed to happen 
during dynamic initialization stage, but any compiler I know of performs it in 
static initialization. In case if there exists such a bizarre compiler, you 
can always resort to runtime checks.

Also I should add another note to my review.

2.15. ctti<T>::n() should support C++11 __func__ standard macro and not fail 
when it's available. You can fail to parse it, but the code should compile and 
return __func__ values from raw_name(), name() and pretty_name().