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: Antony Polukhin (antoshkka_at_[hidden])
Date: 2014-04-27 14:33:09

2014-04-27 18:18 GMT+04:00 Andrey Semashev <andrey.semashev_at_[hidden]>:

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

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