I just forgot to add: in the future, an actual comparison of performances - with and without RTTI - with standard or with boost Would be a very nice addition.

On Fri, Apr 25, 2014 at 12:26 AM, Klaim - Joël Lamotte wrote:

On Sun, Apr 20, 2014 at 8:14 PM, Niall Douglas wrote: I was wondering though: - what is the behaviours of pretty_name() when the type is in namespaces (there are no example of this); - what is the behaviours of pretty_name() when the type is in an anonymous namespace?

I had similar questions reading the doc as well. Or the name of a template full specialization? But what bothered me a little was the note about the fact that pretty_name() was not consistent across platforms/compilers. This means you can't depend on it for generating a type-name-based factory registration via a template taking the type to be instantiated, and deducing the registration name from the type, in a way that will be cross-compiler or cross-platform, w/o some kind of post-processing to remove possible struct/class prefix, or namespace prefix if desired. It's only a problem when the names are coming at runtime from external strings, but that's a common enough use-case of factories, to want boost::typeindex to support it, no? Thus I'd request a safe_name() / consistent_name() method, that does the pretty_name() post-processing once and for all, to yield ns_name::type_name for example, or perhaps even a std::pair with NS name as .first, and type name as .second. Thanks, --DD

On Fri, Apr 25, 2014 at 10:22 AM, Antony Polukhin wrote:

2014-04-25 11:25 GMT+04:00 Dominique Devienne :

...

Or the name of a template full specialization?

You didn't reply to that one. Would we get/see the angled brackets and commas for example? What about nested template arguments? Nested brackets?

...

But what bothered me a little was the note about the fact that pretty_name() was not consistent across platforms/compilers.

Unfortunately this definitely won't be fixed in nearest releases.

OK, I understand. But please note that Boost has a history of smoothing out compiler/platform differences, and that it's actually a large part of its success IMHO. So this is a bit disappointing to me, but of course it is by no means a -1 vote. I'm +1 on boost::typeindex, definitely.

Making a generic solution will consume too much time for each compiler and anyway there's no portable way of doing so for anonymous namespaces.

As noted above, I view Boost as *the* portable way to such facilities, which of course must be implemented specifically for different compilers (although I suspect the patterns are similar across compilers, so an 80% solution that works on the major compilers would be good enough for me).

There is a an ability to make your own type_indexes, so this could be fixed by user manually.

But that defeats the "purpose" IMHO. Sure, we build on far fewer compilers and OSs than Boost at large, so it might be easier to do for us than for you, but the 80% solution I mention above could at least be included at some point, no? In any case, thanks for all your efforts on boost::typeindex. The doc is quite nice already. Thanks, --DD

On Friday 25 April 2014 09:25:45 Dominique Devienne wrote:

But what bothered me a little was the note about the fact that pretty_name() was not consistent across platforms/compilers.

This means you can't depend on it for generating a type-name-based factory registration via a template taking the type to be instantiated, and deducing the registration name from the type, in a way that will be cross-compiler or cross-platform, w/o some kind of post-processing to remove possible struct/class prefix, or namespace prefix if desired. It's only a problem when the names are coming at runtime from external strings, but that's a common enough use-case of factories, to want boost::typeindex to support it, no? Thus I'd request a safe_name() / consistent_name() method, that does the pretty_name() post-processing once and for all, to yield ns_name::type_name for example, or perhaps even a std::pair with NS name as .first, and type name as .second.

There is generally no way to obtain a portable type name. Each compiler and platform has its own mangling rules and there are no rules for the demangled type names whatsoever. Attempting to support translation from all platform- specific formats to some "unified" format is hardly feasible, IMHO. The best way of achieving your goals is to avoid using type names altogether and use regular strings in known format, UUIDs or whatever other IDs that have stable format.

On Sunday 27 April 2014 13:02:41 Rob Stewart wrote:

On April 27, 2014 10:43:27 AM EDT, Andrey Semashev wrote:

...

On Friday 25 April 2014 09:25:45 Dominique Devienne wrote:

...

But what bothered me a little was the note about the fact that pretty_name() was not consistent across platforms/compilers.

There is generally no way to obtain a portable type name. Each compiler and platform has its own mangling rules and there are no rules for the demangled type names whatsoever.

The language standard specifies rules for the demangled names, does it not?

No. The type_info::name() method returns some implementation-defined string with no guarantees on its format. __func__ also has unspecified format. To my knowledge, there are no other sources of type name strings in the language.

On Monday 28 April 2014 05:48:42 Rob Stewart wrote:

On April 27, 2014 1:47:22 PM EDT, Andrey Semashev wrote:

...

On Sunday 27 April 2014 13:02:41 Rob Stewart wrote:

...

On April 27, 2014 10:43:27 AM EDT, Andrey Semashev

wrote:

...

...

On Friday 25 April 2014 09:25:45 Dominique Devienne wrote:

...

But what bothered me a little was the note about the fact that pretty_name() was not consistent across platforms/compilers.

There is generally no way to obtain a portable type name. Each

compiler

...

...

and platform has its own mangling rules and there are no rules for the demangled type names whatsoever.

The language standard specifies rules for the demangled names, does

it not?

No. The type_info::name() method returns some implementation-defined string with no guarantees on its format. __func__ also has unspecified format. To my knowledge, there are no other sources of type name strings in the language.

I wasn't clear. I meant that the language grammar specifies the syntax of the demangled names, though it permits lots of whitespace variation.

The problem is that those strings are not required to follow that syntax. And they don't in some cases.

On Monday 28 April 2014 06:03:16 Rob Stewart wrote:

On April 28, 2014 5:54:41 AM EDT, Andrey Semashev wrote:

...

On Monday 28 April 2014 05:48:42 Rob Stewart wrote:

...

On April 27, 2014 1:47:22 PM EDT, Andrey Semashev

wrote:

...

...

On Sunday 27 April 2014 13:02:41 Rob Stewart wrote:

...

On April 27, 2014 10:43:27 AM EDT, Andrey Semashev

wrote:

...

...

On Friday 25 April 2014 09:25:45 Dominique Devienne wrote: > But what bothered me a little was the note about the fact that > pretty_name() was not consistent across platforms/compilers.

There is generally no way to obtain a portable type name. Each

compiler

...

...

and platform has its own mangling rules and there are no rules for

the

...

...

...

...

demangled type names whatsoever.

The language standard specifies rules for the demangled names,

does

...

...

it not?

No. The type_info::name() method returns some implementation-defined string with no guarantees on its format. __func__ also has unspecified

format.

...

...

To my knowledge, there are no other sources of type name strings in the language.

I wasn't clear. I meant that the language grammar specifies the

syntax of

...

the demangled names, though it permits lots of whitespace variation.

The problem is that those strings are not required to follow that syntax. And they don't in some cases.

If they have the required information, however it may be encoded, all that's needed is to resemble it. Is that out of scope?

As I said, the format of the strings is not defined. You may be able to reconstruct something resembling a C++ type declaration or you may not. In some cases there simply isn't a C++ syntax to represent a type name (e.g. types with anonymous namespaces). The only way to deal with it is to support every particular compiler out there and hope it provides enough information in the strings. A task I called not feasible and not particularly useful.

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.

...

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

...

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.

...

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

...

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.

...

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

...

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.

...

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.

...

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

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

2014-04-30 18:56 GMT+04:00 Andrey Semashev :

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?

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

2014-05-01 13:07 GMT+04:00 Andrey Semashev :

...

2014-04-30 18:56 GMT+04:00 Andrey Semashev :

...

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

On Wednesday 30 April 2014 19:40:54 Antony Polukhin wrote: 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.

Currently, in some cases std::type_info is the argument. If type_index is the argument in those cases, linker errors will protect from crashes in run time. But these functions are not always used by the application, so it's not a 100% protection.

In other cases std::type_info const* is stored in structures and containers thereof, and do not affect mangled type and function names. I'll have to inspect the code to see if Boost.Log actually has these cases but it certainly does not prohibit them; they may appear in user's code as well.

I know there are workarounds for this. Like add yet another tag to Boost.Log internal namespace. (BTW, using a function is not really a good solution because this function needs to be called in many places of the code; it reduces efficiency, introduces false dependency on the binary, and is easy to miss). But these are workarounds that are not needed now and required to support a controversial feature. With Boost.Log's internal namespace the workaround may seem easy to implement, but other libraries don't have such a namespace, and there the fix would not be so easy.

Can we make somehow such link-time assertion in TypeIndex library itself (extern variable, function...)? I have no good ideas right now, but I'm almost certain that it is possible.

...

...

...

...

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?

In that header, I can see the code that works around the problem by using template specializations. As I understand, the compiler matches specializations for int when the template is instantiated for signed int. Why not make the same workaround in Boost.TypeIndex? The case is very much the same.

I do not have any will to add many overloads to fix an antique compiler that even can not be tested. How about a following workaround for those compilers: typename boost::mpl::if_< boost::is_signed, boost::make_signed, boost::mpl::identity

::type::type res_t;

-- Best regards, Antony Polukhin

2014-05-01 21:54 GMT+04:00 Andrey Semashev :

On Thursday 01 May 2014 21:02:14 Antony Polukhin wrote:

<...>

...

How about a following workaround for those compilers:

typename boost::mpl::if_< boost::is_signed, boost::make_signed, boost::mpl::identity

...

::type::type res_t;

And then use typeid(res_t)? Yes, that'd be ok, but it's better to do it like this:

typename mpl::eval_if< mpl::and_< is_integral, is_signed >, make_signed, mpl::identity

...

::type res_t;

At least MSVC 7.1 has problems applying is_signed to non-integral types, not sure about those EDG compilers, but it's better to be on the safe side.

This must be fixed in Boost.TypeTraits. It's better to fix it once instead of puttung workarounds all around the Boost. -- Best regards, Antony Polukhin

2014-05-02 7:35 GMT+04:00 John Bytheway :

On 2014-05-01 13:02, Antony Polukhin wrote:

...

2014-05-01 13:07 GMT+04:00 Andrey Semashev :

...

...

2014-04-30 18:56 GMT+04:00 Andrey Semashev

...

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

On Wednesday 30 April 2014 19:40:54 Antony Polukhin wrote: 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.

Currently, in some cases std::type_info is the argument. If type_index is the argument in those cases, linker errors will protect from crashes in run time. But these functions are not always used by the application, so it's not a 100% protection.

In other cases std::type_info const* is stored in structures and containers thereof, and do not affect mangled type and function names. I'll have to inspect the code to see if Boost.Log actually has these cases but it certainly does not prohibit them; they may appear in user's code as well.

I know there are workarounds for this. Like add yet another tag to Boost.Log internal namespace. (BTW, using a function is not really a good solution because this function needs to be called in many places of the code; it reduces efficiency, introduces false dependency on the binary, and is easy to miss). But these are workarounds that are not needed now and required to support a controversial feature. With Boost.Log's internal namespace the workaround may seem easy to implement, but other libraries don't have such a namespace, and there the fix would not be so easy.

Can we make somehow such link-time assertion in TypeIndex library itself (extern variable, function...)? I have no good ideas right now, but I'm almost certain that it is possible.

MSVC has #pragma detect_mismatch for this purpose (introduced in a relatively recent version):

http://msdn.microsoft.com/en-us/library/ee956429.aspx

I don't know whether similar solutions are available in other compilers.

Thanks a lot! Did not know about this feature. Looks like Clang has the same pragma: http://clang.llvm.org/doxygen/classclang_1_1PragmaDetectMismatchHandler.html Though I have not found a clang version from which this feature is supported. -- Best regards, Antony Polukhin