Re: [OpenMethod] review starts on 28th of April

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2025-05-06 14:20:59

On Sun, 27 Apr 2025 at 15:15, Дмитрий Архипов via Boost
<boost_at_[hidden]> wrote:
>
> Dear Boost community. The peer review of the proposed Boost.OpenMethod will
> start on 28th of April and continue until May 7th. OpenMethods implements open
> methods in C++. Those are "virtual functions" defined outside of classes. They
> allow avoiding god classes, and visitors and provide a solution to the
> Expression Problem, and the banana-gorilla-jungle problem. They also support
> multiple dispatch. This library implements most of Stroustrup's multimethods
> proposal, with some new features, like customization points and
> inter-operability with smart pointers. And despite all that open-method calls
> are fast - on par with native virtual functions.

Hi all,

This is my formal review of the proposed Boost.OpenMethod. Thanks
Jean-Luis for proposing the library, and Dmitry for managing the
review.

I'll follow the points suggested by the review manager. I've numbered
my comments to make replying easier.

> * What is your evaluation of the design?

1. The library achieves the goal of emulating open methods without
language support, which is quite impressive. The amount of macros in
the interface makes it a little bit dirty, but I guess that's
unavoidable when trying to emulate a language feature.

I have to say I'm not a big fan of open methods because they move a
lot of checks that usually happen at compile time to runtime. But I
acknowledge that this pays off with extra flexibility. Taking into
account the popularity of yomm2, the predecessor of the proposed
library, it looks like open methods fit a real need.

2. That said, I think that policies and facets would benefit from some
re-designing.
a. Aside from rtti, the other facets have no formal specification on
their requirements. This means that there is no formal API for these
types. This makes it prone to breaking users in subsequent releases,
and may block evolution by fearing breaking users.
b. The design heavily relies on static members in policies and facets,
which are effectively global variables. It then adds a set of tools
and techniques (CRTP, basic_policy::add, basic_policy::fork) to deal
with the problems caused by the former. I think that these problems
can be avoided altogether by redesigning the system.
c. Policies have two functions: stating how to do things (i.e. stating
that vptrs should be stored in a vector) and acting as a type registry
(i.e. allocating the concrete vector for vptrs as a static member). I
think that this breaks separation of concerns. Policies should only
state how to do things, and a separate "type registry" entity should
be in charge of creating the required static variables required to
register types.

I've already suggested a possible design for this in a previous
message (https://lists.boost.org/Archives/boost/2025/05/259451.php),
so I won't repeat it here. If the library ends up being accepted, I
can make a PR with the proposed changes.

3. The boost_openmethod_vptr customization point does not seem to play
well with policy scoping. Given a type to be inspected, T, the
signature is:

std::uintptr_t boost_openmethod_vptr(const T&);

There is no way to know which policy T was registered with. This
function normally uses Policy::static_vptr, so this is a problem.
Ideally, it should be redesigned to cope well with policy scoping.

4. Compile-time errors are difficult to interpret. For example, at one
point, I wrote:

BOOST_OPENMETHOD(print, (virtual_<base_node>), void);

This is a mistake because the argument to virtual_ should be a
reference type. This is a snippet of the compiler error I got:

"error: no member named 'peek' in
'boost::openmethod::detail::parameter_traits<boost::openmethod::virtual_<base_node>,
custom_policy>'"

I had to look at the implementation to see what was the problem, and
it took me some time to figure it out.

I think that this problem can be alleviated by using C++20 concepts in
places like virtual_. Since the library's minimum standard is C++17,
you'll need some macro machinery to make this work. Other libraries
like Asio do it with good results.

5. virtual_<T&>, with T& not being polymorphic, compiles. In the
program I was writing, this was a programming error - I had forgotten
a virtual destructor. This behavior seems to exist to support
"programs that call methods solely via virtual_ptrs created with the
"final" construct", as stated in minimal_rtti's docs. When would I
want to do this? Does this not defeat the purpose of using open
methods themselves?

6. The <boost/openmethod.hpp> file is unusual. It does not include all
the headers in the library, and it pours definitions in the global
namespace. I'd advise against doing this, because it's not what most
users expect. My recommendation is to:
a. Rename this file to <boost/openmethod/keywords.hpp> or any other
name that you think it's helpful.
b. Make <boost/openmethod.hpp> include all headers except for keywords.hpp.

Another possibility would be to create a namespace
boost::openmethod::keywords that contains using directives for
virtual_, virtual_ptr and similar pseudo-keywords. The user can then
consume these with a using namespace directive, similar to how custom
literals are consumed.

7. boost::openmethod::type_id is a type alias for std::uintptr_t. I
would try to avoid such an alias, since it doesn't add much and makes
code more difficult to read. It might be misleading given the
existence of the typeid operator.

* What is your evaluation of the implementation?

I haven't examined it in detail. Aside from my comments on static
members, the pieces I looked into seemed good.

CIs would benefit from some extra coverage - at the moment, only the
latest compiler versions, with their default standard C++, seem to be
checked.

* What is your evaluation of the documentation?

It contains enough information to cover the basic use cases, but it
needs work. These are my main comments:

8. I think that the idea of splitting the docs into a "basic" and an
"advanced" section, as proposed by the author before, is great and
should happen.

9. I dislike the single-page architecture. I know that Boost.Unordered
(https://www.boost.org/doc/libs/1_88_0/libs/unordered/doc/html/unordered/intro.html)
uses Antora to render asciidoc as multiple pages - maybe it's a
technology that's worth to explore.

10. Facets need a formal API - a section that documents what members
are required and which signatures should they have.

11. In general, some concepts seem to be used without being introduced first:
a. Overrider. When I first read through the docs, I assumed it meant
"the set of functions that implement an abstract method". When I
reached the description of BOOST_OPENMETHOD_OVERRIDER, I saw that it
refers to a concrete C++ struct with a concrete naming scheme and set
of members. I think that this should be explained better, before
BOOST_OPENMETHOD_OVERRIDER is introduced.
b. Domain. By reading the code, it refers to a subset of the type
registry. It's used in the reference (e.g. "debug extends release but
it does not fork it. Both policies use the same domain").
c. "virtual_ptr's "final" constructs". Mentioned in the docs without
defining what they are.

12. The custom RTTI example uses a virtual destructor. This is
important because the default policy's is_polymorphic implementation
uses std::is_polymorphic, but I don't think it's explained. I don't
know if having a virtual destructor is common in custom RTTI
scenarios, but I'm inclined to think that it's not, as AFAIK a virtual
destructor adds standard RTTI information to a type. Clang's AST nodes
don't have it. I'd advise to add an example on how to support a custom
RTTI scheme without virtual destructors, as it wasn't trivial for me
to implement it.

More minor points at the end of this email.

* What is your evaluation of the potential usefulness of the library?

I don't have a use case for it, and I wouldn't use it in my projects,
since I think it adds too much complexity for the benefit it brings.
However, considering the traction that its predecessor, yomm2, has, I
think it would be useful in Boost.

It's a pity that no standardization is pursued. Being a language
emulation library, it looks like eventual standardization should be a
goal. But I understand that it might not be feasible at this point.

* Did you try to use the library? With what compiler? Did you have any problems?

I built one of the examples and played with it. I then wrote a toy
example to check how an API like clang's AST could integrate with this
library. I had some problems at first, due to my lack of understanding
of the library design and the amount of checks that move to runtime.
But I was successful in the end. With some documentation improvements,
it'd be enough to be able to use the library without problems even in
advanced use cases. I used clang 20, C++23, with CMake's Debug
configuration, in an Ubuntu 24.04 system.

* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I've spent around 15h reading the documentation, building and playing
with examples, reading parts of the source code, asking questions and
writing this review.

* Are you knowledgeable about the problems tackled by the library?

I heard about open methods at the author's talk at using std::cpp
2024. I hadn't used them before this review. I have designed and used
class hierarchies and OOP before.

* Affiliation disclosure

I am currently affiliated with the C++ Alliance.

* Final verdict

Although I'm not fully convinced, my recommendation is to ACCEPT this
library into Boost, as I think Boost is better with than without it. I
strongly recommend redesigning the facet/policy system, but I won't
make it an acceptance condition because I don't know if what I propose
is fully viable.

* Minor documentation comments

13. Code in the dynamic loading section looks a little bit too C. In
particular, I'd try to avoid strcat (vs. strncat), since it leads to
bad programming practices that can end up in buffer overruns.
14. In the description of the RTTI policy, in the type_name function:
"This function is optional; if it is not provided, "type_id(type)" is
used.". I'd phrase it as "the numeric type_id is inserted into the
stream, as per stream << type". I originally thought it was a typo and
that typeid(type).name() was used.
15. I'd try to avoid the "bom" alias, in favor of "namespace
openmethod = boost::openmethod;".
16. The std_rtti::type_index reference says that the return value is
unspecified, and then it states that it returns a std::type_index.
17. Reference docs for use_classes mention Policy as an extra template
argument, which is not what the actual code looks like. Instead, the
passed arguments are inspected, and if one is a policy, it is used as
the class registry. This should be noted in the docs. It is also not
clear what happens if several policies are specified in a single
use_classes instantiation.

Regards,
Ruben.