[typeindex v3.0] Peer review begins Mon 21st ends Wed 30th
Dear Boost, A second round of community review begins for proposed Boost.TypeIndex on Mon 21st, lasting ten days. I should apologise as it's my fault for starting another peer review straight after Boost.Align ends, however I was busy as Google Summer of Code admin until today, and I will be preparing to talk at C++ Now straight after this review ends. I hope that this doesn't affect the community's willingness to contribute to the review. Reminder of the report from the last TypeIndex v2.1 review: https://groups.google.com/forum/#!topic/boost-list/TeiSdkRkUF0 Reminder of the original peer review announcement: http://boost.2283326.n4.nabble.com/TypeIndex-Peer-review-period-for-li brary-acceptance-begins-ending-Thurs-21st-Nov-td4654788.html Source code: https://github.com/apolukhin/type_index/zipball/master Github: https://github.com/apolukhin/type_index Documentation: http://apolukhin.github.com/type_index/index.html Changes in TypeIndex v3.0 since v2.1 (from my own review of it): 1. Documentation is hugely improved over v2.1, lots of side by side examples illustrating the differences and similarities. 2. You can now create your own custom type indices using an extensible framework. 3. boost::typeind::type_info now exports raw_name() and pretty_name() to maximise MSVC-GCC compatibility, and to solve point 1 but not point 2 in the v2.1 report. 4. The undefined behaviour trick used in v2.1 has been replaced with a cast via the placeholder type boost::typeind::detail::ctti_data which can be viewed in https://github.com/apolukhin/type_index/blob/master/include/boost/type _index/ctti_type_index.hpp. There is an explanation in the comments there about this technique's compatibility with the C++ standard. 5. TypeIndex now defines everything in the boost::typeind namespace instead of the boost namespace. So: 1. What is your evaluation of the design? 2. What is your evaluation of the implementation? 3. What is your evaluation of the documentation? 4. What is your evaluation of the potential usefulness of the library? 5. Did you try to use the library? With what compiler? Did you have any problems? 6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? 7. Are you knowledgeable about the problem domain? 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. Thanks, Niall -- Currently unemployed and looking for work in Ireland. Work Portfolio: http://careers.stackoverflow.com/nialldouglas/
-----Original Message----- From: Boost-users [mailto:boost-users-bounces@lists.boost.org] On Behalf Of Niall Douglas Sent: 20 April 2014 19:14 To: boost@lists.boost.org; boost-users@lists.boost.org; boost- announce@lists.boost.org Subject: [Boost-users] [typeindex v3.0] Peer review begins Mon 21st ends Wed 30th
Dear Boost,
A second round of community review begins for proposed Boost.TypeIndex on Mon 21st, lasting ten days. I should apologise as it's my fault for starting another peer review straight after Boost.Align ends, however I was busy as Google Summer of Code admin until today, and I will be preparing to talk at C++ Now straight after this review ends. I hope that this doesn't affect the community's willingness to contribute to the review.
Reminder of the report from the last TypeIndex v2.1 review: https://groups.google.com/forum/#!topic/boost-list/TeiSdkRkUF0
Reminder of the original peer review announcement: http://boost.2283326.n4.nabble.com/TypeIndex-Peer-review-period-for-li brary-acceptance-begins-ending-Thurs-21st-Nov-td4654788.html
Source code: https://github.com/apolukhin/type_index/zipball/master
Github: https://github.com/apolukhin/type_index
Documentation: http://apolukhin.github.com/type_index/index.html
Changes in TypeIndex v3.0 since v2.1 (from my own review of it):
1. Documentation is hugely improved over v2.1, lots of side by side examples illustrating the differences and similarities.
2. You can now create your own custom type indices using an extensible
framework.
3. boost::typeind::type_info now exports raw_name() and pretty_name() to maximise MSVC-GCC compatibility, and to solve point 1 but not point 2 in the
report.
4. The undefined behaviour trick used in v2.1 has been replaced with a cast via the placeholder type boost::typeind::detail::ctti_data which can be viewed in https://github.com/apolukhin/type_index/blob/master/include/boost/type _index/ctti_type_index.hpp. There is an explanation in the comments there about this technique's compatibility with the C++ standard.
5. TypeIndex now defines everything in the boost::typeind namespace instead of
v2.1 the
boost namespace.
So:
1. What is your evaluation of the design? OK. 2. What is your evaluation of the implementation? OK.
Though I'm not enthusiastic about the name of namespace boost::typeind:: but I don't have any much better ideas. Perhaps someone else has? Nor am I clear that this namespace bti helps the reader. "Through all the examples, we'll assume that the following namespace alias is in effect: namespace bti = boost::typeind; " IMO, the frequency of use of the namespace bti is probably so low that spelling it out is clearer, for example: boost::typeind::type_id<T>().pretty_name() // human readable
3. What is your evaluation of the documentation?
As advertised - Very Much Improved ! Looks nice and C++ Reference section works for me. Examples use code snippets so can be sure that the code shown will compile and run.
4. What is your evaluation of the potential usefulness of the library? Nice to have. 5. Did you try to use the library? No. 6. How much effort did you put into your evaluation?
A quick reading of the docs.
7. Are you knowledgeable about the problem domain? Not specially. 8. Do you think the library should be accepted as a Boost library?
Yes. Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 01539 561830
2014-04-23 17:33 GMT+04:00 Paul A. Bristow <pbristow@hetp.u-net.com>: <...>
2. What is your evaluation of the implementation? OK.
Though I'm not enthusiastic about the name of namespace boost::typeind:: but I don't have any much better ideas. Perhaps someone else has?
Nor am I clear that this namespace bti helps the reader.
"Through all the examples, we'll assume that the following namespace alias is in effect:
namespace bti = boost::typeind;
"
IMO, the frequency of use of the namespace bti is probably so low that spelling it out is clearer, for example:
boost::typeind::type_id<T>().pretty_name() // human readable
Agreed: boost::typeindex would be more user friendly. It suggests to the reader which library to look for. Other notes will be also addressed. Thanks for the review! -- Best regards, Antony Polukhin
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... [2] https://sourceforge.net/p/turtle/code/729/ [3] http://sourceforge.net/p/turtle/code/HEAD/tree/trunk/test/detail/test_type_n...
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... [2] https://sourceforge.net/p/turtle/code/729/ [3] http://sourceforge.net/p/turtle/code/HEAD/tree/trunk/test/detail/test_type_n... _______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
-- Currently unemployed and looking for work in Ireland. Work Portfolio: http://careers.stackoverflow.com/nialldouglas/
2014-04-26 13:07 GMT+04:00 Mathieu Champlon <m.champlon@free.fr>:
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
Hi, Here is my review of the Type Index library. I have not studied version 2.1, so everything is entirely based on v3.0
1. What is your evaluation of the design?
Seems reasonable to me. It is an enhancement of std::type_info with additional functionality and methods. I think the most useful function is pretty_name(). There have been some discussion about the namespace, I also do not like boost::typeind. boost::typeindex would be fine for me. I have one suggestion: Can e method for splitting the name into a part for the namesapce and a part for the type name be provided? Or even a range of (nested) namespace names. I could imagine something like type_id< A::B::Foo >.type_name() prints "Foo" and type_id< A::B::Foo >.namespace_name() returns a range with ["A","B"]
2. What is your evaluation of the implementation?
I searched the code to find the place where the name in pretty_name is generated, and I found __cxa_demanlge :). No seriously, I have not looked very detailed into the implementation.
3. What is your evaluation of the documentation?
Documentation is fine. Maybe an example with the output of pretty_name() could be put into the Getting started section, to see immediately the advantage of this library. And an example with namespaces would be great, showing that namespaces are also correctly displayed.
4. What is your evaluation of the potential usefulness of the library?
For me it is really useful. I have two use cases where I would immediately use this library, one is showing the name of expections and the other is giving better error messages in a type erased library.
5. Did you try to use the library? With what compiler? Did you have any problems?
Yes, I played around with it for 1-2 hour. Works out of the box, with gcc-4.7 and gcc-4.8. There where no problems, everything works smoothly.
6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
1-2 hour with playing with examples and reading the docs.
7. Are you knowledgeable about the problem domain?
A bit.
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, I think it should be accepted.
2014-04-30 17:22 GMT+04:00 Karsten Ahnert <karsten.ahnert@googlemail.com>:
Hi,
Here is my review of the Type Index library. I have not studied version 2.1, so everything is entirely based on v3.0
1. What is your evaluation of the design?
Seems reasonable to me. It is an enhancement of std::type_info with additional functionality and methods. I think the most useful function is pretty_name(). There have been some discussion about the namespace, I also do not like boost::typeind. boost::typeindex would be fine for me.
I have one suggestion: Can e method for splitting the name into a part for the namesapce and a part for the type name be provided? Or even a range of (nested) namespace names. I could imagine something like
type_id< A::B::Foo >.type_name() prints "Foo" and type_id< A::B::Foo >.namespace_name() returns a range with ["A","B"]
This could be useful but I'd rather not put it in this library. Such functionality is not widely requested and can be easily implemented manually using Standard library and type_id<T>().pretty_name(). If there'll be a way to add more reflection functionality I'd make a separate library.
2. What is your evaluation of the implementation?
I searched the code to find the place where the name in pretty_name is generated, and I found __cxa_demanlge :). No seriously, I have not looked very detailed into the implementation.
3. What is your evaluation of the documentation?
Documentation is fine. Maybe an example with the output of pretty_name() could be put into the Getting started section, to see immediately the advantage of this library. And an example with namespaces would be great, showing that namespaces are also correctly displayed.
OK, I'll fix that.
4. What is your evaluation of the potential usefulness of the library?
For me it is really useful. I have two use cases where I would immediately use this library, one is showing the name of expections and the other is giving better error messages in a type erased library.
5. Did you try to use the library? With what compiler? Did you have any problems?
Yes, I played around with it for 1-2 hour. Works out of the box, with gcc-4.7 and gcc-4.8. There where no problems, everything works smoothly.
6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
1-2 hour with playing with examples and reading the docs.
7. Are you knowledgeable about the problem domain?
A bit.
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, I think it should be accepted.
Thank you for the review! -- Best regards, Antony Polukhin
participants (1)
-
Antony Polukhin -
Karsten Ahnert -
Mathieu Champlon -
Niall Douglas -
Paul A. Bristow