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

From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2025-04-29 16:35:52

El 27/04/2025 a las 15:15, Дмитрий Архипов via Boost escribió:
> Dear Boost community. The peer review of the proposed Boost.OpenMethod will
> start on 28th of April and continue until May 7th.

This is my review of the Boost.OpenMethod proposal. Thanks to Jean-Louis 
for
his work and to Dmitry for managing!

As a guide to this review, I'm answering the points proposed by the 
review manager
succintly and then a full review section is given at the end with much 
more detail
(in no particular order).

* What is your evaluation of the design?

I find it adequate and well thought out, maybe leaning on overcomplexity for
the sake of covering as much as possible (below).

* What is your evaluation of the implementation?

I didn't look closely, but what little I saw looked to me robust. Code 
looks clean, too.

* What is your evaluation of the documentation?

Quite extensive and probably covers anything that needs be covered, but
I have qualms about it (below).

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

It is very useful if you need open methods, obviously. Personally I haven't
delved much into the world of virtual functions (I'm more of a template
programmer), but the fact that the predecessor of this library (YOMM2)
has real users is enough to convince me that the proposal fills a need.

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

Yes, to get my head around it by writing small snippets. VS 2022.
No problems detected.

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

I spent 11 hours playing with the library and reading and re-reading
the docs.

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

I'm familiar with open methods since I read about them in Alexandrescu's
2001 "Modern C++ Design" (where they were called multimethods).
I think I'm sufficiently familiar with virtual function machinery to 
understand
the technical challenges met by this proposal and the solutions adopted.

* Do you think the library should be accepted as a Boost library?

Yes, I vote to ACCEPT Boost.OpenMethod. I have some
observations/reservations about it, some of them more important than others.
To be clear, I'm not conditioning my vote on these observations being
addressed, since I know the author will ponder them carefully and respond
in the most appropriate way --which may not be the way I suggest.

FULL REVIEW

Main points:

* The docs are quite hard to read (to me, at least). Mainly, I miss clear
introductions and explanations of the many concepts involved before using
them in the tutorial. A (probably incomplete) list of concepts that I had to
wrap my head around is:
 Â  + virtual_ptr, double role as a fat reference and as a virtual 
argument marker
 Â  + virtual_
 Â  + compiler
 Â  + guide function
 Â  + policies and facets

* Policies, in particular, are introduced in a way that feels to me 
haphazard
and incomplete. I never can be sure if I have all the policy-related 
information
correctly and exhaustively.

* I’d encourage the author to try and remove from the docs (i.e. not 
disclose
publicly) as many utilities as possible – I have the feeling some 
components are
described that don’t really belong in the public API, though I may be 
wrong on
this one.

* I think virtual_ptr is a misnomer and the entity should be renamed to
virtual_ref or something similar.

* I challenge the need to have virtual_shared_ptr, virtual_unique_ptr 
and the
like. In my opinion, the proposal need not conflate virtual arguments with
object lifetimes, the latter being orthogonal to the purpose of the library.

* The decision to call an arbitrary override when there’s a tie-in looks 
erroneous
to me, and it may at least be controlled by some policy facet.

* The library relies very heavily on macros, and the macro-free alternative
does not seem too practicable. I guess this can’t be helped, but it’d be 
nice
if some thought were given to how to alleviate this overdependence on 
macros.

Elaboration and full list of observations:

* virtual_ptr. I have a number of problems with this class:
 Â  + The documentation starts using it right away without telling the 
user what it
is about. At first, I thought it was merely a syntactic marker to 
indicate wher
a virtual argument happens, which is the case, but not the whole story.
 Â  + Its semantics are not those of a pointer, but behave more like a 
reference.
This is explicitly acknowledged later in the docs, on the grounds that 
virtual_ref,
which is a more apt name, is reserved for potential evolutions of the C++
language to include overloading of the dot operator. I think it is extremely
unlikely that this C++ feature will ever be realized.
 Â  + As it happens, an open method works equally well if a reference to 
a class is
passed instead of a virtual_ptr (this is apparent as soon as one 
realizes that
virtual_ptr can be constructed from a plain reference, but I didn’t read 
that far
into the reference). In a private communication with the author, he told 
me this
is indeed the case but virtual_ptr is more efficient because it stores 
an extra
direct pointer to the associated virtual table, in a manner not 
dissimilar to what
Rust’s fat pointers do (again, this is mentioned later in the 
reference). I was led
astray by the implicit assumption that virtual_ptr was a sort of pointer,
which is not.
   + What’s the point if having virtual_ptr() and virtual_ptr(nullptr_t) 
constructors
if virtual_ptr behaves as a reference? Same with the move constructor, 
what’s the
point of emptying the source virtual_ptr?
 Â  + virtual_ptr<const C> is constructible from virtual_ptr<C> but not 
the other
way around, as it should be. This is not readily apparent from reading the
reference, though.
 Â  + All in all, I strongly recommend that virtual_ptr be renamed to 
virtual_ref, and
that an early explanation in the docs is given for its presence as 
opposed to using
plain references. operator -> can be retained because it provides a 
terse syntax
to access the underlying object, but consider providing as well some value()
member function and/or a conversion operator to the underlying object.

* “Multiple and virtual inheritance are supported, with the exception of
repeated inheritance”: I don’t know what repeated inheritance means –I may
make an educated guess, but I don’t think this is standard terminology.

* Instead of BOOST_OPENMETHOD(poke, (std::ostream&, 
virtual_ptr<Animal>), void),
consider the possibility of using
BOOST_OPENMETHOD(poke, void(std::ostream&, virtual_ptr<Animal>)).

* When virtual_ptr is used, classes are required to be polymorphic (i.e. 
classes
with a virtual table), which is perfectly ok. But if I fail to declare 
any virtual
function in my class hierarchy, simple snippets of code still compile and
produce spurious results. It would be good if this could be caught and 
signaled
by a compile-time error.

* policies:debug and policies:release: I understand that these do not mix
together? I mean, if a DLL is compiled in debug mode, its exported open
methods won’t be taken by an executable compiled in release mode, right?
Not sure if this is expected and/or desireable.

* “If there is still no unique best overrider, one of the best 
overriders is chosen
arbitrarily.” I’m not sure this is the best default option, probably a 
run-time
error is more appropriate here, maybe controlled by some facet of the 
policy.

* In the description of the release policy, it’s stated that, for 
instance, the facet
extern_vtpr is implemented with vptr_vector and vtpr_map. This doesn’t make
much sense to me, as a policy can’t possibly provide more than one
implementation for a given facet. Looking at the source code I learnt 
that it’s
vptr_vector that’s used for extern_vptr in release.

* “If BOOST_OPENMETHOD_DEFAULT_POLICY is defined before including this
header, its value is used as the default value for the Policy template 
parameter
throughout the code. Otherwise, boost::openmethod::default_policy is used.”
I think it woud be more correct to say “Otherwise,
BOOST_OPENMETHOD_DEFAULT_POLICYÂ  is defined to
boost::openmethod::default_policy”.

* In the reference, initialize is declared as:

template<class Policy = BOOST_OPENMETHOD_DEFAULT_POLICY>
auto compiler<Policy>::initialize() -> /*unspecified*/;

I understand that this should be

template<class Policy = BOOST_OPENMETHOD_DEFAULT_POLICY>
auto initialize() -> /*unspecified*/;

* “OpenMethod supports dynamic loading on operating systems that are
capable of handling C++ templates correctly during dynamic link.” I don’t
understand what this means, and what operating systems are those
(All the usual ones? Or is it a rare feature?)

* I miss a complete specification of what facets and other pieces of info
a policy can/must have, what happens when some particular facet/piece
of info is not explicitly provided, and what facets/pieces of info can be
user-defined instead of simply chosen from the available catalog of options
in the library.

* The reference includes lots of small classes such as type_id, 
vptr_type, etc.
but it’s not clear to me if these are implementation details or things for
which a user can provide alternative implementations within a policy. If the
former, I’d remove them from the reference.

* The section “Core API” explains how to implement the “poke” open method
without utility macros. But then the example uses “poke::fn(…)” rather than
plain “poke(…)”. What additional step is needed to achieve “poke(…)” syntax
as macro-supported examples accomplish? (Peeking into the code, I see that
BOOST_OPEN_METHOD includes the definition of a free-standing forwarding
function inline auto NAME(ForwarderParameters&&... args)…)

* Core API: the example uses BOOST_OPENMETHOD_REGISTER, it would be
nice to show how to get rid of all macros, including this one.

* A virtual argument in an open method can be defined with virtual_ptr<C> or
virtual_<C>. I understand why these two exist--the former is more efficient.
But then we also have virtual_shared_ptr and virtual_unique_ptr: what’s the
advantage of using virtual_shared_ptr<C> instead of
std::shared_ptr<virtual_ptr<C>>? This seems to add some clutter to the 
lib for
no real benefit, plus it conflates the concepts of “virtual slot” and
“object lifetime”, which in my mind are completely orthogonal, the 
latter not
really belonging in this library, much like classical virtual functions 
are not
concerned with object lifetime either. FWIW, this extension is not part 
of N2216.

* Elaborating on the previous point, a cleaner approach (IMHO) would be to
mark virtual slots with virtual_<C> exclusively, and then let this 
interoperate
smoothly with virtual_ptr <C> (or virtual_ref if the name is finally 
changed).
That is, virtual_ is concerned with the signature of open methods only, and
virtual_ptr is concerned with run-time optimization of reference passing,
only.

* I understand that BOOST_OPENMETHOD_DECLARE_OVERRIDER and
BOOST_OPENMETHOD_DEFINE_OVERRIDER separate the declaration and
definition that are covered together in BOOST_OPENMETHOD_OVERRIDE.
But what does BOOST_OPENMETHOD_OVERRIDER do? The explanation in
the reference is quite opaque to me.

* The docs talk about so-called guide functions, but I can’t seem to 
find this
concept defined anywhere.

* Regarding <boost/openmethod.hpp>, it’s stated that it “[a]lso imports
boost::openmethod::virtual_ptr in the global namespace. This is usually
regarded as bad practice. The rationale is that OpenMethod emulates a
language feature, and virtual_ptr is equivalent to keyword, similar to
virtual.” I agree this is bad practice, I’d recommend against it.

* I can define and override an open method inside a namespace (say open
method run my_namespace), so that’s ok. But I can’t override outside the
namespace with BOOST_OPENMETHOD_OVERRIDE(my_namespace::run,…).
At least, this should be mentioned as a limitation of the macro-based 
interface.

* I know that an internal type_hashed is required because of previous
communications with the author around this very point, but I have the 
feeling
this piece of the lib is not adequately explained in the docs and a casual
reader may be confused by it.

Joaquin M Lopez Munoz