[Boost] [Describe] library submission - last call for reviews from the community
A friendly reminder that the review period for [Boost.Describe] will finish at the end of tomorrow - 10th March 2021 We've had some excellent feedback already, and would value more. If you can spare a few hours of your valuable time, please consider reviewing this neat little library and posing your results on this mailing list. The invitation is repeated below: The Boost formal review of the Describe starts Monday, taking place from March 1st, 2021 to March 10th, 2021 (inclusive). The library is authored by Peter Dimov. Documentation: https://pdimov.github.io/describe/doc/html/describe.html Source: https://github.com/pdimov/describe The library provides a simple means of providing reflection for structures and enums in C++. The documentation contains a number of common motivating examples, including: - A universal print function https://pdimov.github.io/describe/doc/html/describe.html#example_print_funct... - JSON serialisation https://pdimov.github.io/describe/doc/html/describe.html#example_to_json Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including Describe as a Boost library. Please be explicit about your decision (ACCEPT or REJECT). Some other questions you might want to consider answering: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement. Thank you for your efforts in the Boost community. They are very much appreciated. Richard Hodges - review manager of the proposed Boost.Describe library Peter is often available in the CppLang Slack #boost channel should you require any clarification not covered by the documentation, as am I. Please submit your review to the Boost mailing list: boost@lists.boost.org -- Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212
> Please provide in your review information you think is valuable to
> understand your choice to ACCEPT or REJECT including Describe as a
> Boost library. Please be explicit about your decision (ACCEPT or REJECT).
CONDITIONAL ACCEPT
> Some other questions you might want to consider answering:
>
> - What is your evaluation of the design?
A bit unwieldy but ok in general. Only mentioning the places I'd look
for improvements:
- BOOST_DESCRIBE_ENUM_CLASS seems unnecessary. Would be great, if that
could go as IIRC E::e works also for unscoped enums.
- I'm surprised by the complexity of the runtime loop over enumerators.
The need for another type is odd and makes me wonder about similar
usages in user/library code and whether that could be simplified
- BOOST_DESCRIBE_STRUCT is likely most often used for PODs without base
classes. Having to add the empty base class list could be removed and
auto-detected
- The limitation of protected/private base classes not being
distinguishable seems odd for a Boost-quality library which have a
history of pushing odd-case boundaries. Detection of protected base
classes IMO is possible, see https://godbolt.org/z/GfWhGf
- The modifiers bitfield is a potential trap, as noted in the review of
Julien Blanc. I understand the reasoning and agree with it, however I'd
like to see inline-comments like "default: just non-static" or something
like that.
- Missing support for overloaded functions and reference members might
be serious. I understand the problem with not being able to create
pointers to reference members. I'm wondering how that could be supported
in a compiler based introspection and if the should be already "solved"
here. Overloads are much more serious IMO and IIRC there is a solution
to disambiguate overloads. I'd like to see at least a road to
implementation that does not change the interface to something
incompatible.
> - What is your evaluation of the implementation?
"Usual" template magic, nothing surprising for the most part. So great work!
- Limitation to 24 members seems a bit low, especially for enums
- All tests are silently skipped if C++(14) support is not good enough
("silently" as in b2 they will be reported as "run" although nothing is
done)
- requirement for gnu++14 on GCC < 8 is also very odd for a Boost lib.
I'm not sure what is supposedly missing here, but at least the enum
stuff seems to fundamentally work at least: https://godbolt.org/z/n3foGn
- nit: IIRC we had a convention to not put macros inside the boost namespace
- Some insource documentation would be great, at least for the macros
with variadic arguments. As mentioned in another review developers
rather use GoToDefinition than reading docs ;)
- I'm missing some error checking. E.g. we use a switch-case based
enum-to-string conversion generated by a PP_FOREACH macro, which is
basically BOOST_DESCRIBE_ENUM, but there the compiler warns about
missing enumerators whereas something like that seems to be impossible
here. Not sure if this is possible in this design but wanted to mention
this as with the DESCRIBE macros stuff can become out-of-sync
- some functions (e.g. enum_descriptor_fn_impl) are not constexpr. Looks
like an oversight to me, if not a short comment nearby would be great to
help other devs in the future.
> - What is your evaluation of the documentation?
Also great.
However as Mp11 is basically required a very(!) short summary of the
functions commonly used/required for using this library would help. Also
some examples concerning the modified usage for class members showing
the difference between "additional" and "toggle" like semantics.
> - What is your evaluation of the potential usefulness of the library?
Very useful, especially to pave the road to standardization of such
reflection
A bit concerned about potential for bugs by not updating class/enum
definitions and define macros in real world code
> - Did you try to use the library? With which compiler(s)? Did you
> have any problems?
No, just read examples, code and some godbolt experiments
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
See above. ~5h
> - Are you knowledgeable about the problem domain?
A bit, mostly the subset of enums: Researched and developed enum
reflection solutions. Mostly Iteration over enumerators and some macro
based enum-to-string conversion generation.
My conditions would be:
- Full support for -std=c++14, not just the GNU variant unless proven to
be technically impossible and then (more) selectively disabling the
problematic stuff. We have that in other cases with macros like
BOOST_NO_XXX as currently it seems a lot of the library is
removed/stubbed as the general C++14 support is disabled for GCC < 8
- Support for protected vs private bases
- In-source docu for the most important parts (macros, modifiers)
Reasoning: IMO that is what is expected of Boost-quality libraries:
Works even for "buggy" compilers. Supporting only the gnu variant for
GCC <=7 is quite limitting
Similar the protected base class issue (IMO) can be solved and if it
really can not, then trying to get protected base classes should fail to
compile at least instead of giving wrong (empty) results.
Regards,
Alex
Alexander Grund wrote:
- All tests are silently skipped if C++(14) support is not good enough ("silently" as in b2 they will be reported as "run" although nothing is done)
I changed that recently on purpose, as the library is intended to be "usable" (that is, not cause compilation errors) if types are described under C++11. The describe_* primitives don't return anything then, of course, but this makes it possible to BOOST_DESCRIBE types without having to #ifdef.
- requirement for gnu++14 on GCC < 8 is also very odd for a Boost lib. I'm not sure what is supposedly missing here, but at least the enum stuff seems to fundamentally work at least: https://godbolt.org/z/n3foGn
The ##__VA_ARGS__ extension is not supported. See https://godbolt.org/z/hc3eEv. The default for g++ 6/7 when no -std is given is gnu++14, so I expect this to cause less trouble than one would ordinarily expect. Of course it would have been better for the library to just work, but I don't know how to fix it. (I've already learned way too much preprocessor programming than the WHO recommends.) Thank you for the other observations, they are appreciated. (And thanks to all other reviewers, of course.)
Am 10.03.21 um 18:44 schrieb Peter Dimov via Boost:
Alexander Grund wrote:
- All tests are silently skipped if C++(14) support is not good enough ("silently" as in b2 they will be reported as "run" although nothing is done) I changed that recently on purpose, as the library is intended to be "usable" (that is, not cause compilation errors) if types are described under C++11.
The describe_* primitives don't return anything then, of course, but this makes it possible to BOOST_DESCRIBE types without having to #ifdef. I'm personally not a fan of that being the default especially as there are now many levels: C++98, C++11, GNU++14, C++14 and each silently enables parts of the library.
I kinda understand the motivation: Likely being able to pull in external, C++11/14 libs (headers) into C++98/11 libs, but I think the use case is too narrow: Likely other parts won't work But this comment was on tests: Why not skip tests altogether if the required features are not supported just as done with pretty much any other test? And add (compile) tests for C++98/11 mode (or actually all) where only the macros are used? To empathize: My complaint was, that it is currently indistinguishable if a test succeeded or was completely ignored due to C++ requirements.
- requirement for gnu++14 on GCC < 8 is also very odd for a Boost lib. I'm not sure what is supposedly missing here, but at least the enum stuff seems to fundamentally work at least: https://godbolt.org/z/n3foGn The ##__VA_ARGS__ extension is not supported. See https://godbolt.org/z/hc3eEv.
The default for g++ 6/7 when no -std is given is gnu++14, so I expect this to cause less trouble than one would ordinarily expect. Of course it would have been better for the library to just work, but I don't know how to fix it. (I've already learned way too much preprocessor programming than the WHO recommends.) That's why I argued to use the BOOST_* feature test macros (or own ones if new ones are required). E.g. you also disabled the enum stuff although it works just fine: https://github.com/pdimov/describe/blob/eea288e04961374721020b5454707590815b... https://godbolt.org/z/n3foGn
This could be avoided if you would have used BOOST_NO_VARIADIC_EXPAND_FEATURE (or so) which would allow to see, that this feature is not used for the enum case. This is meant as a suggestion for improvement to make this even more useful. I experimented a bit with that and found a PoC that it does work with GCC < 8 under at least some constraints: https://godbolt.org/z/9WxnbG Or https://godbolt.org/z/PEKofd There might be more options, see e.g. https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/ Although the default is gnu++ the majority of software installations I've seen do add -std=c++xx in their configure scripts (if they check at all) and in the project I'm maintaining we use the CMake setting to disallow the GNU stuff. I'm also actually surprised, that it works on MSVC without ##__VA_ARGS__ . I though their preprocessor is now standards conformant...?
Alexander Grund wrote:
I kinda understand the motivation: Likely being able to pull in external, C++11/14 libs (headers) into C++98/11 libs, but I think the use case is too narrow: Likely other parts won't work
The motivation, as I said, is for people to be able to BOOST_DESCRIBE their types without making their libraries require C++14. E.g. for Boost.JSON to be able to BOOST_DESCRIBE this enum: https://github.com/boostorg/json/blob/develop/include/boost/json/kind.hpp#L2...
Alexander Grund wrote:
E.g. you also disabled the enum stuff although it works just fine: https://github.com/pdimov/describe/blob/eea288e04961374721020b54547 07590815b2fcf/include/boost/describe/enum.hpp#L12 https://godbolt.org/z/n3foGn
The problematic case is BOOST_DEFINE_ENUM(A), which I actually don't handle properly even on newer compilers. It only compiles because of another problem - that a trailing comma is silently accepted and produces an invalid descriptor. BOOST_DEFINE_ENUM_CLASS(A) fails, and so will the enum one, once I unify the two macros.
This could be avoided if you would have used BOOST_NO_VARIADIC_EXPAND_FEATURE (or so) which would allow to see, that this feature is not used for the enum case.
It's not used, but by mistake. :-)
I'm also actually surprised, that it works on MSVC without ##__VA_ARGS__ . I though their preprocessor is now standards conformant...?
MSVC implicitly does the ## thing without being told. Their newer preprocessor, enabled with /Zc:preprocessor, does not (as pointed out by Edward Diener), but it supports the ## extension.
The problematic case is BOOST_DEFINE_ENUM(A), which I actually don't handle properly even on newer compilers. It only compiles because of another problem - that a trailing comma is silently accepted and produces an invalid descriptor. BOOST_DEFINE_ENUM_CLASS(A) fails, and so will the enum one, once I unify the two macros. IMO that case is not problematic because you only would need to differentiate between BOOST_DEFINE_ENUM(A) and BOOST_DEFINE_ENUM(A, xxx) but not BOOST_DEFINE_ENUM(A,). This is possible without any extension: https://godbolt.org/z/1qrrn4 Another point could be made that enums without enumerators are simply unsupported as I fail to see a use case.
In either case the GNU extension is not required so I see no reason to not support enums
It's not used, but by mistake. :-)
My point was that the more specific macro name allows to see that more easily. E.g. I had to ask, which GNU extension you need as that isn't mentioned anywhere in the code I have less of an issue with the CXX11 macro, although I'd likely call it BOOST_DEFINE_ENABLE or so, but that's bikeshedding.(Actually: see below)
MSVC implicitly does the ## thing without being told. Their newer preprocessor, enabled with /Zc:preprocessor, does not (as pointed out by Edward Diener), but it supports the ## extension.
The motivation, as I said, is for people to be able to BOOST_DESCRIBE
Does this mean the library will break in that mode? Or could you always use the ## extension? (Didn't understand if only the new PP supports it) their types without making their libraries require C++14. Sure, makes sense, I don't mind as I have since then realized, that calling the functions will fail to compile. So only "issue" is, that mistyped/outdated stuff in the macros will not appear until compiled with C++11(14?). On that matter: The library is C++14, isn't it? I'm confused by the BOOST_DESCRIBE_CXX11 macro. What is it used for? My impression is, that the library cannot be used in C++11 mode at all. (I.e. call functions), so that macro doesn't make much sense to me Similar to my actual point here (tests being compiled and run as No-Ops instead of not at all) I don't understand the tests. E.g. https://github.com/pdimov/describe/blob/eea288e04961374721020b5454707590815b... Why are there conditions on C++11 and 14 if the macros are supposed to work/compile in C++98? Unless I have misunderstood something my proposal would be: - 1 macro to enable the C++14 parts which do not require the ## extension (e.g. the enum code) - 1 macro to additionally enable the parts which require that extension (and C++14 of course) - specific names for that (i.e. not simply *_CXX14 because that extension isn't 14 [some of that is actually in 20 IIRC]) - Use B2 to enable/disable tests
participants (3)
-
Alexander Grund -
Peter Dimov -
Richard Hodges