From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2021-03-10 12:28:23
> 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