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

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 :

...

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.