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

2014-04-28 12:11 GMT+04:00 Andrey Semashev :

On Sunday 27 April 2014 22:33:09 Antony Polukhin wrote:

...

2014-04-27 18:18 GMT+04:00 Andrey Semashev :

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