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