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: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2014-04-26 11:32:02

Many thanks Joel and Mathieu for your reviews.

Niall

On 26 Apr 2014 at 11:07, Mathieu Champlon wrote:

> 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
> _______________________________________________
> Boost-users mailing list
> Boost-users_at_[hidden]
> http://lists.boost.org/mailman/listinfo.cgi/boost-users

-- 
Currently unemployed and looking for work in Ireland.
Work Portfolio: http://careers.stackoverflow.com/nialldouglas/