[Boost] [Describe] Review

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