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: Antony Polukhin (antoshkka_at_[hidden])
Date: 2014-04-26 12:08:43

2014-04-26 13:07 GMT+04:00 Mathieu Champlon <m.champlon_at_[hidden]>:

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

I wanted to highlight the differences between getting information of a
known type and determinating type info at runtime. That's why we have

bti::type_id_runtime(variable) and bti::type_id<Type>()

Otherwise `bti::type_id(variable)` may lead to some misunderstanding: Are
we getting real type of variable or it's bti::type_id<decltype(variable)>()
?

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

This is an optimization trick. Some of the dynamic loaders place same
symbols at same addresses. So if we have "std::vector<int>" string in two
different dynamic loadable libraries, there is a chance that both strings
will have same addresses. I've looked into the sources of strcmp. It does
not check for lhs == rhs.

raw_name() does not return NULL.

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

GCC, Clang and MSVC do not warn. I see no problems in such shadowing.

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

All those issues will be fixed.

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

Thanks for the review and useful comments!

-- 
Best regards,
Antony Polukhin