On Fri, Mar 5, 2021 at 8:19 AM Richard Hodges via Boost < boost@lists.boost.org> wrote:
The Boost formal review of the Describe [...] The library is authored by Peter Dimov.
Thank you Peter for proposing this library.
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).
ACCEPT.
Some other questions you might want to consider answering: - What is your evaluation of the design?
Simple enough to describe the types. And macros can't be helped apparently. Some reviewers have complained about the DEFINE_ENUM macros. I'm fine with them, and would have liked to have them a few years back; would have saved me from creating my own inferior macro. They are welcome conveniences, to avoid repeating the enumerators. To close the ENUM chapter, I did run into two issues: https://github.com/pdimov/describe/issues/4 https://github.com/pdimov/describe/issues/6 I'm not sure what can be done about the first one. I copy/pasted/edited my enum definition, which uses trailing commas, and macros don't like trailing commas. Was clearly my mistake, but I certainly wasted time on this one. Which was compounded by the limit on 24 enumerators that blindsided me. I expect Peter will resolve the latter somehow, or at worse document the limit. I also opened https://github.com/pdimov/describe/issues/8, where I misused the STRUCT macro for an ENUM, and got no error. Easy to static_assert for that I suspect, although may not be worth it. Not for me to decide, misuse is misuse after all. My experimentation with Boost.Describe were based on generated code, using an existing code generator modified to create Peter's type descriptors, for a dozen enums, and about 200 structs for a custom network protocol, and I made a simple error during generation, thus this weird find. On the STRUCT side, I initially struggled because of namespaces. Again, I was not playing with Peter's examples directly, but trying to integrate directly Boost.Describe in my existing code, generated or not, with the hope to replace a part of it with Boost.Describe. And the generated structs happen to be in different namespaces, which can complicate matters compared to the simpler (in terms of namespaces) examples provided by Peter. I entered https://github.com/pdimov/describe/issues/7 and once again Peter helped me get past the build errors I got. I added universal printing (op<<) and generated op== for my 200+ structs, something I was missing before. The equality example Peter added during the review, which is not part of the doc, should probably be added, as it's generally useful IMHO (if one's not on C++20?) Once again, it was mostly user-error. The documentation clearly states the descriptors need to be in the described type's namespace (they are), but I was not putting the templates using the descriptors in that same namespace, and things got a bit "complicated" then. Maybe there should be a troubleshooting section in the doc, when encountering compiler errors. Easier said than done though, each case and/or compiler will be different I expect. Still, I feel like it's the biggest impediment to a successful use of Boost.Describe, as it's not easy to get past those build errors, and it might be off-putting to many "normal" developers like me.
- What is your evaluation of the implementation?
I did not look at the implementation. I do not feel qualified to review it, nor do I have the time, honestly.
- What is your evaluation of the documentation?
The documentation is very good. The language is clear. And does look good too. I've read it both on a large screen on desktop, and on a tablet. Several times :) Something I would have liked to see, was a section explaining in more details the template metaprogramming necessary when using the descriptors and MP11. For example, during the review it was mentioned SFINAE automatically removes from consideration non-described types when trying to instantate the templates using the descriptors, thus avoiding ambiguities, but the doc does not mention this. The print-an-enum examples, at compile-time versus runtime, give an inkling of the differences between the two worlds, but I feel a gentle more beginner-friendly explanation of what's going on, or links to outside doc, to better understand the concepts of a lambda with auto argument being "called" by a compile-time for-loop, for devs unfamiliar with these things, would be helpful. Not everyone is at that level of understanding, including me. There's also no section on compile-time performance in general, or the overhead of using BOOST_DEFINE_ENUM, versus a plain non-described enum. If that enum ends up included in many TUs, how much is that "costing" me? All my descriptor macros are in a separate file, i.e. explicit opt-in, to not force clients to #include Boost.Describe and its Boost.MP11 dependency. Am I overly cautious here? Without more details on compile-time performance, hard to say, except to try it, but on my cross-platform large "legacy" codebase, that's not convenient. I'd prefer the library doc giving my hard numbers, with 100, 1000 TUs with and w/o Boost.Describe used for example (The above is why I strongly disagree with forcing to use STRUCT macros inside the struct, like CLASS macros require, as I don't want all client-code to see those macros and necessary dependencies, unless they really need to)
- What is your evaluation of the potential usefulness of the library?
Very useful. I'm still not done fully leveraging it, but I'm certainly well on my way to use it in production. The ability to decouple the graph traversal of structs for operations done on them (e.g. serialization) is my end goal, and super useful, since in my case I need different types of serialization (and op== for unit testing). I had prototyped a Boost.Serialization-like solution, but Boost.Describe looks like a promising replacement. But again, I'm not done with that in time for this review to report success on the custom serialization aspects.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
MSVC 2019 in C++17 mode. GCC 9.1 in C++17 mode. No particular problems, except my own failings and resulting build issues, as outlined above.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Maybe 10-15 hours over several days, excluding the time to update my code generator. I've read the doc many times. And worked out build issues (with Peter's help) on my own "examples", based on Peter's examples.
- Are you knowledgeable about the problem domain?
On the enum side, somewhat, having developped (with help from this ML) a similar solution for enum-to-string, and string-to-enum several years ago. But the template to use descriptors are quite new to me, and I would have been lost without the examples. Sorting out build errors from template-voodoo like Boost.Describe requires is the challenge for devs like me, to be honest. When it builds, it works flawlessly and magically. When it doesn't, well, good luck! It's not this library's fault of course. I'm not sure anything can be done about it. I think that's it. My first Boost Review. Thanks, --DD