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