On 20/04/2014 20:14, Niall Douglas wrote:

1. What is your evaluation of the design?

Is there any reason why bti::type_id_runtime(variable) could not be bti::type_id(variable) ? Is it to make it stand out or is there a technical limitation ?

2. What is your evaluation of the implementation?

Here is what I noticed:

I find the name type_index_facade a bit misleading as a facade to me refers to the design pattern which obviously isn't what it is about.

(in type_index_facade.hpp)

    inline bool equal(const Derived& rhs) const BOOST_NOEXCEPT {
        return raw_name() == rhs.raw_name() || !std::strcmp(raw_name(), rhs.raw_name());
    }
(and similarly in before(...))

Why first test with == ? Won't strcmp take care of it?
Or is it to filter out the invalid cases where raw_name() returns a null pointer? Are we sure that either both or none of them can be null?
Just below, hash_code doesn't seem to check when calling std::strlen.
Maybe raw_name() is supposed to always return a non-null pointer? In that case it should be added to the function documentation.

Doesn't type_index_facade::type_info(), type_index_facade::raw_name() etc.. shadow the same functions in the derived classes?
I don't know if that could be a problem like some compilers issuing a warning?

3. What is your evaluation of the documentation?

It's quite nice overall, I noted a few glitches:

. I find the phrasing of "And that is the point, where problems start" in the motivation section a bit awkward. To be honest I had to re-read it a few times before I could understand what it meant.
I think something much simpler would be easier to read like ", which is where problems start"

. with a quick glance at the motivation section it wasn't obvious to know if Boost.TypeIndex fixes the const/ref/volatile stripping issue with the compilers which don't implement it correctly or if it levels the differences by always stripping them
Again I think a simpler phrasing would help, maybe "some implementations of `typeid(T)` erroneously strip const, volatile and references from type" which would imply that Boost.TypeIndex fixes this

. the section "Making own type_index" could probably be renamed "Customizing type_index" or "Making a custom type_index"

Well, English not being my first language I may just be delirious here. :)

I found the "How to use" section very useful as this allowed me to quickly replace hand-written code with Boost.TypeIndex
However it took me a couple of minutes to understand what please_save_modifiers and with_crv meant, and I'm not sure all C++ developers out there now that crv stands for const reference volatile... Maybe adding a C++ style comment explaining each line would help. Also links to each function documentation would be nice.

The doxygen comment for type_id_runtime has a typo in "/// Retunrs runtime information about specified type." (should be Returns)
A little further the documentation for runtime_val has a similar one (Varaible instead of Variable)
Isn't there some way to run the doc through a spell checker to catch these?

The documentation of type_id_runtime doesn't explain how it behaves in respect to cvr qualifiers?

    // noexcept comparison operators for type_index_facade classes.
    bool operator?(const type_index_facade & lhs,
                   const type_index_facade & rhs);

Is this a doxygen glitch? Should there be a note stating '?' stands for '==', '!=', etc..?

4. What is your evaluation of the potential usefulness of the

library?

I believe the motivation section speaks for itself: this library is indeed useful.
My personal interest lies only in retrieving debugging/diagnosis information which oddly seems to also be the case for most of the other reviewers so far (I didn't expect that!).

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

I replaced my hand-crafted type pretty name extraction [1] with boost::typeindex in about 10 mn [2].
All my tests [3] compile and run without a glitch on MSVC2010 and gcc 4.6.3. I just removed one of them which was testing the pretty name of a type local to a function (obviously this doesn't work with pre-C++11 compilers).
The code was based on BOOST_SP_TYPEID and an ad hoc formatting filter. As others have stated I believe this latter feature could be a worthwhile addition to Boost.TypeIndex.

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

A bit of time to integrate it into my library and to write a few tests in order to better understand how the library behaves, about 2h in total.
I somewhat carefully read the documentation and browsed the code a bit.

7. Are you knowledgeable about the problem domain?

Not much past extracting pretty names for logging, and only for MSVC and gcc...

And finally, every review should answer this question:

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.

Yes!

Thanks for writing the library Antony.