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: Antony Polukhin (antoshkka_at_[hidden])
Date: 2014-04-30 08:58:43

2014-04-28 12:11 GMT+04:00 Andrey Semashev <andrey.semashev_at_[hidden]>:

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

rtti_type_info sounds bad to me, especially because it can be used with
rtti off.

Think of the type_info names as of "how it is implemented" not "how it
works":
stl_type_info - implemented using default (Standart Library) type_info
ctti_type_info - implemented using specific compile time tricks.

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

Then it's an ability of the user to define the desired behavior. I think
that it is better to be more explicit - if user uses pretty_name() of its
own type_info, then the user wish to distinguish pretty_name() from 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.
>

I'd agree to the following:
* new section describing the library extension by deriving from
type_index_facade
* reference section remains unchanged (except for a note that this methods
do not actually exist). Here is why: All the class related stuff
(interface, contracts, guarantees) must be in reference section. That is
the place where people look for answers. It won't be good to spread the
info all around the docs, forcing people to search info on many pages.

>  > > 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 uSo instead of making
> somethingsers. 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.
>

ABI depends on compiler, compiler flags, compiler version, defined
macros... Library maintainer may grantee ABI stability ONLY when nothing of
that was changed. All other cases are users responsibility.

I do not see how this could be a blocker. If user starts mixing binaries
with (for example) _ITERATOR_DEBUG_LEVEL=0 and _ITERATOR_DEBUG_LEVEL=2 -
it's his problems not a put-any-library-name-here.

Though there must be added a note into the docs about the requirement to
recompile binaries in case of defining BOOST_TYPE_INDEX_USER_TYPEINDEX.

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

I'll change the code to work with const char*. In that case this problems
will disappear.

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

pretty_name() must produce same pretty names for same types. When
pretty_name() returns "i" and "int" for same type - that is a total mess.

I'm starting to understand what implementation of pretty_name() you want to
see. You want to have an MSVC like name(), which returns demangled name and
*must* be noexcept. Try out type_index::name() - it's what you need.

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

The only problem is that I'm not sure that such compiler will distinguish
signed int from int. We have no regression testers that can check this out.

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

OK, thanks.

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

OK

-- 
Best regards,
Antony Polukhin