
10 Mar
2021
10 Mar
'21
12:28 p.m.
> 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