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

2014-04-30 18:56 GMT+04:00 Andrey Semashev <andrey.semashev_at_[hidden]>:

> I've rearranged some parts of the original message for better locality.
>
> On Wednesday 30 April 2014 16:58:43 Antony Polukhin wrote:
> > 2014-04-28 12:11 GMT+04:00 Andrey Semashev <andrey.semashev_at_[hidden]>:
> > >
> > > 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.
>
> Could you at least move the methods to the class' description?
>

OK

>  > > 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.
>
> The ABI incompatibilities between release and debug (and different flavors
> of
> debug in case of MSVC) libraries have caused a lot of grief already, even
> though Boost.Build tags debug and release binaries differently. Adding yet
> another source of incompatibilities certainly doesn't make things better.
>
> Boost.Log has a compiled part. In some cases it needs to pass RTTI between
> the
> compiled part an the user's application. If the application defines
> BOOST_TYPE_INDEX_USER_TYPEINDEX then Boost.Log becomes broken. Hoping that
> the
> user never does that is not what I'm prepared to do (because he will, and
> will
> report a bug about weird crashes in Boost.Log). That is why I won't be
> able to
> use your library in Boost.Log.
>

I see your point. Let's try to resolve this somehow without throwing away
the BOOST_TYPE_INDEX_USER_TYPEINDEX macro.

Is in your case type_index one of the function arguments? If yes - then
user won't be able to link modules with and without
BOOST_TYPE_INDEX_USER_TYPEINDEX. Linker won't find functions.

If no - can we make some fake function that has type_index argument and
does nothing (just verifies the type_indexes compatibilities)?

>  > >  > >     - 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().
>
> I think interface consistency is more important in this case. After all,
> that's one of the reasons why you provide type_index_facade. I'll expand
> on it
> below.
>

You've convinced me.

>  > > > > 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.
>
> That's a good point. However, I can see three sources of failure:
>
> 1. Memory depletion - either __cxa_demangle or std::string construction
> fails.
> 2. __cxa_demangle fails because the input string is not valid. Hardly a
> valid
> case since the input string is a constant generated by compiler.
> 3. pretty_name() fails to parse the output of __cxa_demangle (e.g. because
> it
> has unexpected format).
>
> I agree that #1 should never be concealed. #2 and #3 either will always
> happen
> or never happen for a given input (mangled) string. I'm pretty convinced
> that
> #3 should not result in a failure (i.e. pretty_name() should return
> normally
> in this case). I'm not so sure about #2, but since that is a more
> theoretical
> problem I'm not concerned about it much. So I guess I'll change this point
> of
> my review to only conceal #3 kind of errors.
>

This sounds reasonable. OK

>  > 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.
>
> No, I'm not trying to make pretty_name() to have MSVC behavior. And it
> can't
> be noexcept since it has to allocate memory. I'll remind my view, which I
> expressed in my first round of review and assumed in the second one:
>
> - raw_name() is a function that returns low-level (mangled, underlying,
> short,
> system, etc.) name of the type.
> - name() returns exactly what std::type_info::name() returns, or whatever
> other equivalent if type_index is not based on std::type_info.
> - pretty_name() returns a string that may be more human readable than
> name()
> or raw_name().
>
> All three functions are part of the formal interface of any type_index
> (RTTI-
> based, CTTI-based or user-defined) and may potentially return the same
> string.
> For example, if a particular platform or a particular type_index
> implementation does not support "prettyfication", pretty_name() can be
> implemented to return name(). This behavior, I believe, should be the
> default
> and implemented in type_index_facade.
>
> If you leave pretty_name() out of the formal interface of type_index then I
> think usefulness of the library is significantly reduced. It can still be
> used
> as a key in associative containers but not for diagnosic purposes. In
> particular that means that you can't use pretty_name() in operator<<.
>
> > > 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.
>
> Then perhaps you shouldn't bother with that check in the first place. Add a
> workaround when someone with such a compiler appears.
>

In both cases we get code that does not work on ancient EDG like compilers.
But it is better to notify user that library does not work at compile time.
Otherwise user will get a hard to debug runtime error.

Information about that issue is taken from here:
https://github.com/boostorg/python/blob/master/include/boost/python/type_id.hpp
Looks like it is not only ints related. If I change the static assert
message to more generic will it be OK for you?

-- 
Best regards,
Antony Polukhin