Re: [boost] [Describe] My review of Describe

pt., 5 mar 2021 o 09:00 Julien Blanc via Boost 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>::value == 6); static_assert(mp_size>::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>::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>::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