2014-04-27 18:18 GMT+04:00 Andrey Semashev
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.
Namespace will be changed. I'm open for discussion becuase none of the proposed variants is perfect. 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
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:
OK, (b) seems serious to me.
- raw_name() is required to be defined in the derived class. There must be no raw_name() function in type_index_facade.
Agreed.
- name() is equivalent to raw_name(), if not re-defined in the derived class. The default definition should be present in type_index_facade.
Already done in that way.
- 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
- type_info() is required to be defined in the derived class. Remove it from type_index_facade.
Agreed
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.
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.
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.
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_()
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.
Will be fixed.
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.
OK
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.
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.
(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.
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()?).
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.
Very good point! That'll be optimized.
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.
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::.
Agreed.
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.
2.11. ctti_data constructors and assignment should be deleted. You can use BOOST_DELETED_FUNCTION macro for that.
Agreed
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.
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().
Agreed
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.
I need to think here. There were some ideas how user could use those macros explicitly... but I do not remember how exactly.
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
. This is reasonable to limit the includes. But in order hash_value() to work, has to be included in the user's code. This has to be documented.
Agree with all the notes. Thanks for helping!
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.
Great thanks for reviewing it! -- Best regards, Antony Polukhin