Boost logo

Boost Users :

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.


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]
> [2]
> [3]
> _______________________________________________
> Boost-users mailing list
> Boost-users_at_[hidden]

Currently unemployed and looking for work in Ireland.
Work Portfolio:

Boost-users list run by williamkempf at, kalb at, bjorn.karlsson at, gregod at, wekempf at