Barrett Adair wrote:

I don't like that I need to call describe_members multiple times to get static, non-static, data, and functions. I would appreciate the addition of mod_data and mod_nonstatic to keep the flags consistent. This is my only acceptance condition.

Thank you Barrett for taking the time to review. The reason the static/nonstatic and data/function members are returned separately is that the type of `pointer` changes. For nonstatic data, it's M T::*. For static data, it's M*. For nonstatic function, it's R (T::*)(A...). For static function, it's R (*)(A...). Since one typically does things with `pointer` in the loop over members, it's rarely the case that you want all of these at once, because the syntactic form of the code manipulating them changes. So I've found it in practice more convenient to separate them by default.

On Tue, Mar 2, 2021 at 10:48 PM Peter Dimov via Boost <boost@lists.boost.org> wrote:

Maximilian Riemensberger wrote:

...

2) Overloaded functions are not supported. Again, what are the reasons for this limitation? Is it fundamentally impossible?

The problem is basically that &X::f doesn't work when f is overloaded. There are probably ways around that, such as f.ex. using something like

(int (X::*)(float) const, f)

in place of just `f` (ugh), but I haven't yet settled on a solution I'd be comfortable with.

(Actually I know what solution I'd be comfortable with - adding __describe_members to clang, gcc and msvc - but that's a difficult endeavor.)

By coincidence I was reading Folly's Poly [1] yesterday, where Eric uses folly::sig to support overloading. Not sure it's relevant though :) Which brings up the question of support in Describe of non-member functions, also mentioned on that page. Can they be incorporated into the described members? Just curious. On another topic, could you have an example using std::variant? If a described struct contains a variant which is a mix of other described structs and std:: values (string, vector, optional, etc...), how one does go about dispatching to a custom serialization method? And if that variant is not of the std:: variety, but in fact a discriminated boost::any wrapper, as generated by Avro's C++ code generator, how does one hook into Describe's machinery? Or is it the other way around, and Describe just hands over that weird value, and it's the code it calls that's supposed to be aware of it? (the latter means it's the N operations that must be aware of it, instead of the 1 Describe/Description) Yet on something else, Boost.PFR seems to give an operator== for one's structs out of the box. That's a binary operation, while most of your examples are unary, no? How would "generated" op== work in Boost.Describe? Assuming the described structs don't have themselves an op==, and one would want to generate those op== using their "descriptions"? Thanks, --DD [1] https://github.com/facebook/folly/blob/master/folly/docs/Poly.md

Dominique Devienne wrote:

Yet on something else, Boost.PFR seems to give an operator== for one's structs out of the box. That's a binary operation, while most of your examples are unary, no? How would "generated" op== work in Boost.Describe? Assuming the described structs don't have themselves an op==, and one would want to generate those op== using their "descriptions"?

Here's an example of a generated operator==: https://gist.github.com/pdimov/35bd34dab981667777ff6433818e8768

pt., 5 mar 2021 o 09:00 Julien Blanc via Boost <boost@lists.boost.org> 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

Andrzej Krzemienski wrote:

pt., 5 mar 2021 o 09:00 Julien Blanc via Boost <boost@lists.boost.org> napisał(a): ...

...

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.

That is correct. I already gave a partial answer in https://lists.boost.org/Archives/boost/2021/03/250949.php, but that may have been too short. The majority of use cases deal with nonstatic data members, so the library makes this the default (you don't need to supply any modifier in order to get the nonstatic data members.) In contrast, there's no good default for accessibility. Some use cases need just the public members, some need public+protected, some need all, some need public+protected and separately the private ones, and so on. The cases where one needs the static members are rarer. And since the type of the pointer returned in the descriptor differs between static and nonstatic, one typically doesn't need both returned at the same time. In the rarer still cases where one does want both static and nonstatic members at the same time, there's always the option of mp_append-ing the two descriptor lists. So the design approach here is to keep the more common cases short, while leaving advanced uses more complex. There are many ways one can redesign the modifiers, and I already went through most of them. This is what I have settled on. I do accept the feedback that this needs better documentation, and perhaps comments in modifiers.hpp for the people who look there first (don't we all).

pt., 5 mar 2021 o 17:39 Peter Dimov via Boost <boost@lists.boost.org> napisał(a):

Julien Blanc wrote:

...

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.

BOOST_DESCRIBE_CLASS goes inside the class definition for two reasons: one, to be able to access private members; two, to support class templates. You can do

#include <boost/describe.hpp>

template<class T> struct X { T m;

BOOST_DESCRIBE_CLASS(X, (), (m), (), ()) };

int main() { using namespace boost::describe; using D = describe_members<X<int>, mod_public>; }

(except it seems it doesn't work on MSVC 2017, which I currently use for unrelated reasons, but it does work on g++ and clang++ :-))

In contrast, BOOST_DESCRIBE_STRUCT being put outside the struct allows you to describe structs whose definitions you don't control, such as structs defined in third party libraries and system headers.

So there's a reason for both decisions.

It makes sense to support both cases. Maybe it is just the choice of names. In C++ we are taught that struct is actually a class. Maybe a rename BOOST_DESCRIBE_STRUCT -> BOOST_DESCRIBE_AGGREGATE (or similar) would make the difference easier to spot. Regards, &rzej;