Re: [Boost-users] [typeindex v3.0] Peer review begins Mon 21st ends Wed 30th

Subject: Re: [Boost-users] [typeindex v3.0] Peer review begins Mon 21st ends Wed 30th
From: Mathieu Champlon (m.champlon_at_[hidden])
Date: 2014-04-26 05:07:02

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.

Regards,
MAT.

[1] 
http://sourceforge.net/p/turtle/code/HEAD/tree/trunk/turtle/detail/type_name.hpp
[2] https://sourceforge.net/p/turtle/code/729/
[3] 
http://sourceforge.net/p/turtle/code/HEAD/tree/trunk/test/detail/test_type_name.cpp