Re: [Describe] My review of Describe

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2021-03-05 11:42:15

pt., 5 mar 2021 o 09:00 Julien Blanc via Boost <boost_at_[hidden]>
napisał(a):

> Hi,
>
> Here's my small review of describe :
>
> # TLDR
>
> conditionnaly ACCEPT
>
> # Are you knowledgeable about the problem domain?
>
> Somewhat. I guess every C++ programmer played with various libraries
> that try to provide reflection. In my case, i designed one that has
> been in use in a custom ORM.
>
> # How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
>
> About 10 hours playing with the library. I did *not* look in-depth at
> the library internals.
>
> # With which compiler(s)? Did you have any problems?
>
> Gcc and linux, version 7, 8, and 10. I encountered one issue that will
> be fixed (https://github.com/pdimov/describe/issues/3 ).
>
> With gcc-7, compiling with gnu++14 instead of c++14 is mandatory, as
> stated by the documentation. However, failing to do so results in non-
> understandable error messages. Maybe some improvements could be done
> here.
>
> # What is your evaluation of the design?
>
> The library looks well designed for the describe_enumerators.
>
> There are imho some design flaws, however, with the describe
> struct/class api.
>

Thank you for this review. It expresses a couple of concerns I had but was
not able to put them into words.

> ## Macros
>
> BOOST_DESCRIBE_STRUCT must be put outside the class definition.
> BOOST_DESCRIBE_CLASS, on the other hand, must be put inside the class
> definition. This is typically the "i will always get this wrong" case.
> Since the library heavily relies on templating and macros, the error
> messages are arcane to the final user. I would suggest, that, for
> consistency, BOOST_DESCRIBE_STRUCT should be put inside the class
> definition as well.
>

My impression is that the library addresses a number of use cases that are
fairly unrelated. Of course, they all fall under the category "obtain some
meta-information about the entity''; but other than that I cannot easily
tell the part that is the same about the four use cases.

As an example of this, I cannot answer the question if this library is for
obtaining meta information about already existing definitions, or is it
also for creating new definitions? If it is the former, then
BOOST_DEFINE_ENUM clearly does not belong here. If it is the latter, then I
am clearly missing an analogous definition mechanism for defining structs.

> ## Modifiers
>
> modifiers is defined as enum modifiers
> {
>     mod_public = 1,
>     mod_protected = 2,
>     mod_private = 4,
>     mod_virtual = 8,
>     mod_static = 16,
>     mod_function = 32,
>     mod_inherited = 64,
>     mod_hidden = 128,
> };
>
> which suggests that any combination is valid. That is not the case (and
> not only for the obvious virtual/static). Given the following code:
>
> struct C {
>         int a;
>         std::string b;
>         double c;
>         void f(float a, int b) {
>         }
>         void g() {
>         }
>         static void s() {}
> private:
>         int p_;
>         void h_(std::string const& s) {}
> BOOST_DESCRIBE_CLASS(C, (), (a, b, c, f, g, s), (), (p_, h_))
> };
>
>
> I would expect the following:
> static_assert(mp_size<describe_members<C, mod_public>>::value == 6);
> static_assert(mp_size<describe_members<C, mod_private>>::value == 2);
>
> That's not the case. Instead, the results are resp 3 and 1. By default,
> only data members are returned, not member functions.
>
> So, let's try another one. I want all my member functions:
> static_assert(mp_size<describe_members<C, mod_function | mod_public |
> mod_protected | mod_private >>::value == 4)
>
> --> fails. By default, it only returns non-static functions.
>
> So, let's try to include static function as well:
> static_assert(mp_size<describe_members<C, mod_static | mod_function |
> mod_public | mod_protected | mod_private >>::value == 4)
>
> That also fails. The result is one, now i only get the static
> functions.
>
> It looks that there's a mix of apples and orange in this enum. Some
> values acts as they will include more data in the results, some on the
> contrary will filter, and some will change completely the type of the
> members enumerated. This is not obvious when reading the documentation,
> and there's just nothing in the name of the enumerated values that will
> hints the user about what it really does.
>
> This design issue is my main motivation for the conditional acceptance
> of the library: i think it should not be accepted until this part is
> fixed.
>

I agree with the observation. My explanation for it (I do not know the real
motivation, just saying as a reviewer) is that it is driven by use cases.
Excluding enums, I have counted three:

   - data member (recursive) access for PODs
   - access to all data members (and base-classes) for class types, for the
   purpose of marshalling, etc..
   - member function call by name

For each of these use cases, the library provides a solution that is quite
natural for that specific use case. But each of these use cases expects a
somewhat different interface.

But those three interfaces do not combine to a one coherent whole. I am
missing a programming model here. I realize that what I say is vague. I
cannot provide anything more specific right now.

I would be more comfortable if it offered different functions:

boost::describe::members() // for data members
boost::describe::members_recursive() // decomposes base classes into its
members (possible?)
boost::describe::member_functions() // for functions

> ## Missing features
>
> The library does not provide much. While i understand this is a design
> choice to allow transition to proper reflection when it lands into the
> core language, there are a few things that i believe it could provide,
> which belongs to the "core" of a reflection library.
>
> I'm missing a BOOST_DEFINE_FIXED_ENUM_[CLASS_]FLAG, which would do the
> following:
>
> BOOST_DEFINE_FIXED_ENUM_CLASS(E, Base, v1, v2, …, vN)
>
> enum class E : Base {
>    v1 = 0x1u,
>    v2 = 0x2u,
>    ...
>    vN = 1u << N;
> };
>
> I'm also missing a way to retrieve the base name of the class.
> Something like describe_type<T>::name which would return a char const*
> "T" (≠ typeid(T).name()).
>

Again, I would say that the library does not specify very clearly what its
scope is. In particular, what it is *not*. If it is only for providing
reflection for existing definitions, then I would say it does too much:
BOOST_DEFINE_ENUM is out of scope. But if its scope is to also provide the
definitions (especially for enums), then it is more like a "better enum"
sub-library, and now, as you point out, it provides too little.

BTW, your example should also include overloaded bitwise operators, as they
do not work out of the box for enum classes.

Regards,
&rzej;

> # What is your evaluation of the implementation?
>
> I do not feel qualified enough to evaluate the implementation. It uses
> a lot of preprocessor and template magic, which are far beyond what i
> can evaluate, especially in the time i can spend on this review.
>
> # What is your evaluation of the documentation?
>
> Pretty good. It's clear, looks pretty, and the examples are great. I
> miss a complete reference page, though: something like
> https://think-async.com/Asio/asio-1.18.0/doc/asio/reference.html ,
> which lists all types in the library.
>
> # What is your evaluation of the potential usefulness of the library?
>
> Very useful. In fact, i'm already planning using it everywhere as soon
> as i can get rid of some really old (gcc 4.8) toolchains. Both the enum
> reflection and the member reflection can be of great use in some code
> base i work on. I hope this library will land into boost soon.
>
> Thanks to Peter for this work, which is very valuable.
>
> Regards,
>
> Julien
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>