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-30 10:56:54

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?

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

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

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

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