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
- type_info() is required to be defined in the derived class. Remove it
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
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
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
> 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
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:
# pragma once
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
- 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
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
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
> 4. What is your evaluation of the potential usefulness of the
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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk