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-27 10:18:59

On Sunday 20 April 2014 19:14:13 Niall Douglas wrote:
> Dear Boost,
> 
> A second round of community review begins for proposed
> Boost.TypeIndex on Mon 21st, lasting ten days.
> 
> 1. What is your evaluation of the design?

There are several notes that I think need to be addressed.

1.1. I think the library namespace should be changed from typeind to 
type_indexing (my preference) or type_indices. I think the common practice in 
Boost is to avoid shortened names. Library namespaces are usually a plural of 
the component implemented in the library (atomics, units, flyweights), the 
name of a problem domain (usually, when there's no single component 
implemented by the library: log, math, filesystem) or a sufficiently well 
known abbreviation (mpi, mpl, asio). There are also unrelated names such as 
spirit, fusion and phoenix. In any case, typeind or typeidx do not look like 
good candidates, ti (abbreviation of type index) is too short and ambiguous, 
type_index clashes with the corresponding types in the library and STL, so 
type_indexing (the problem domain) looks the most appropriate. type_indices is 
just a bit ugly.

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.

1.3. Why does type_index_facade define member functions type_info(), 
raw_name(), pretty_name()? The problem with theese functions is that they (a) 
don't add anything to the interface since they have to be re-defined by the 
derived class and (b) result in infinite recursion if the derived class omits 
them. I think there would be more use in these functions if they were defined 
differently and implemented the _missing_ functions in the derived class with 
the default semantics. I can see the following contract:

    - raw_name() is required to be defined in the derived class. There must be 
no raw_name() function in type_index_facade.
    - name() is equivalent to raw_name(), if not re-defined in the derived 
class. The default definition should be present in type_index_facade.
    - 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.
    - type_info() is required to be defined in the derived class. Remove it 
from type_index_facade.

Note that operator<< needs to be updated for this change.

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.

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

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

> 2. What is your evaluation of the implementation?

2.1. Please, avoid calling multiple times the derived methods in 
type_index_facade. E.g. you call raw_name() 3 times in hash_code(). In case of 
std::type_info these calls may actually result in a library call. Just cache 
the first call result and use it. The similar note concerns other places in 
different headers.
2.2. [nitpick] The comment in type_index_facade.hpp says "COMPARISONS with 
Derived" while the comparisons are actually with TypeInfo.
2.3. In fragments like these:

#if defined(_MSC_VER)
# pragma once
#endif

use BOOST_HAS_PRAGMA_ONCE defined in boost/config.hpp.

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

2.7. I would suggest avoiding copying the result of __cxa_demangle into a 
local string just to later return a substring. You can perform the parsing in-
place in the returned buffer and then construct the resulting string from it. 
The automatic memory free can be implemented with a scope guard object instead 
of try/catch.
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>()?
2.9. There is missing #include <cstdlib> in stl_type_index.hpp for 
std::free(). Calls to free() in stl_type_index::pretty_name() should be 
qualified with std::.
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.
2.11. ctti_data constructors and assignment should be deleted. You can use 
BOOST_DELETED_FUNCTION macro for that.
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.
2.13. In ctti_type_index::hash_code(), don't duplicate code of extracting the 
type name from raw_name(). I think the logic I described for pretty_name() 
could be extracted to a private method which returns two pointers (a range) in 
raw_name() which denote the effective type name. That private method can be 
used both in pretty_name() and hash_code().
2.14. Why are there different macros BOOST_TYPE_INDEX_REGISTER_STL_CLASS and 
BOOST_TYPE_INDEX_REGISTER_CTTI_CLASS? I think there's no need for 
BOOST_TYPE_INDEX_REGISTER_STL_CLASS, in case of RTTI emulation 
BOOST_TYPE_INDEX_REGISTER_CTTI_CLASS should always be used (and it should be 
named BOOST_TYPE_INDEX_REGISTER_CLASS). The macro should be defined to nothing 
in case if native RTTI is available.

> 3. What is your evaluation of the documentation?

The docs are quite sufficient for general use. A few minor notes though:

3.1. Please, provide more info on what BOOST_TYPE_INDEX_REGISTER_CLASS does, 
since this intrudes user's classes. At least specify the signature of that 
virtual function so that users can avoid name clashes.
3.2. Getting started: "Unlike Standard Library versions those classes may work 
without RTTI." => "... can work without RTTI."
3.3. Getting through the inheritance to receive a real type name: There's an 
extra quote mark after RTTI.
3.4. Exact type match: storing type with const, volatile and reference. 
Suggest: Exact type matching: storing types with const, volatile and reference 
qualifiers. "In this example we'll create a class, that stores pointer to 
function and remembers the exact type of a parameter that function accepts. 
When an attempt to call the stored function will be made, type of input 
parameter will be checked for exact match with initially erased type of 
function." => "In this example we'll create a class that stores a pointer to 
function and remembers the exact type of the parameter the function accepts. 
When the call to the bound function is made, the actual input parameter type 
is checked against the stored parameter type and an exception is thrown in 
case of mismatch."
3.5. If that was the intention, library extension by deriving from 
type_index_facade should be described in a separate section.
3.6. There needs to be a separate "Configuration" section describing all user-
definable macros and other possible options influencing the library behavior. 
In particular BOOST_TYPE_INDEX_FORCE_NO_RTTI_COMPATIBILITY has to be described 
there.
3.7. In the implementation, you only #include <boost/functional/hash_fwd.hpp>. 
This is reasonable to limit the includes. But in order hash_value() to work, 
<boost/functional/hash.hpp> has to be included in the user's code. This has to 
be documented.

> 4. What is your evaluation of the potential usefulness of the
> library?

Very useful, I've implemented some of its functionality multiple times. I know 
there are places in Boost where some of it is reimplemented.

> 5. Did you try to use the library? With what compiler? Did you have
> any problems?

I did not compile any code.

> 6. How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?

Several hours of reading the code and the docs.

> 7. Are you knowledgeable about the problem domain?

I believe so. I've implemented a similar functionality for Boost.Log and had 
some experience in using std::type_info in different projects.

> 8. Do you think the library should be accepted as a Boost library? Be
> sure to say this explicitly so that your other comments don't obscure
> your overall opinion.

I think the library should be accepted on condition that the following points 
are addressed: 1.1, 1.3, 1.4, 1.5, 2.1, 2.5, 2.6, 2.7, 2.8, 2.9, 2.12, 2.13, 
2.14, 3.1, 3.6, 3.7. I'm open to discussion about these issues but I think 
they significantly affect library usability. The other notes I've pointed out 
in my review are less critical and basically are my wishes for improvement.

I'd like to thank Antony for bringing this library for the second time. I can 
see he has done a great job improving it. Also, thank you Niall for managing 
the review.