[Describe] My review of Describe

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2021-03-05 07:59:39

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.

## 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.

## 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.

## 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()).

# 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