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

From: Jean-Louis Leroy (jl_at_[hidden])
Date: 2025-04-30 22:15:57

Hi Joaquín,

Thank you for the thorough review!

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

Uh, it was hard to write too...

I am going to add a glossary. When the doc stabilizes, I will add
cross-references.

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

Policies are definitely advanced stuff. I found that part of the doc very
difficult to write.

Maybe I should split the doc into "regular" and "advanced" parts.

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

I went the other way, i.e. virtual_ptr now has pointer semantics. I had two
reasons for that.

1. After all these years I realized that the Stroustrup & col papers do allow
   passing virtual arguments by pointer. As far as I can see, that is mentioned
   in a single sentence, and it never appears in any example - only
   pass-by-reference. Still, to be consistent with the papers, I added virtual
   pointer arguments (as in virtual_<Animal*>), and logically, the fat pointer
   followed suit.

2. When virtual_ptr was more ref-like, the interaction with the underlying smart
   pointer was odd. It limited its functionality. Smart pointers can be changed
   and reset. When blended with a virtual_ptr, they lost that. In particular, I
   expected virtual_ptr<std::unique_ptr<T>> to be unusable in many contexts, if
   you cannot detach it from the object it points to.

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

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

That strays from the papers, I know... Here's my reasons...

Consider the case of matrix operations. Most of the time, they will return a new
matrix, allocated on the heap, managed by std::shared_ptr or maybe a
boost::intrusive_ptr. For example, addition returns a new matrix. Using the
N2216 syntax:

    class abstract_matrix {...};
    class ordinary_matrix : abstract_matrix {...};

    auto add(virtual abstract_matrix& a, virtual abstract_matrix& b)
        -> std::shared<abstract_matrix>;

    auto add(virtual ordinary_matrix& a, virtual ordinary_matrix& b)
        -> std::shared<abstract_matrix> {
        return std::make_shared<ordinary_matrix>(...);
    }

 Transposition is interesting. In the general case, it returns a new matrix too.

    auto transpose(virtual ordinary_matrix& m) -> std::shared<abstract_matrix> {
        return std::make_shared<ordinary_matrix>(...);
    }

But for a symmetric matrix, it really looks like we want to return the matrix
itself. Of course we cannot simply do this:

    auto transpose(virtual abstract_matrix& m) -> std::shared<abstract_matrix>;

    auto transpose(virtual symmetric_matrix& m)
        -> std::shared<abstract_matrix> {
        return std::shared_ptr<abstract_matrix>(&m);
    }

...since we are likely to be holding to `m` via a shared_ptr somewhere else. In
this design, there is a simple solution:

    auto transpose_aux(
        virtual abstract_matrix& m, std::shared_ptr<abstract_matrix> shared)
        -> std::shared<abstract_matrix>;

    auto transpose_aux(
        virtual ordinary_matrix& m, std::shared_ptr<abstract_matrix> /*unused*/)
        -> std::shared<abstract_matrix> {
        return std::make_shared<ordinary_matrix>(...);
    }

    auto transpose_aux(
        virtual symmetric_matrix& m, std::shared_ptr<abstract_matrix> shared)
        -> std::shared<abstract_matrix> {
        return shared;
    }

    auto transpose(std::shared<abstract_matrix> m)
        -> std::shared<abstract_matrix> {
        return transpose_aux(*m.get(), m);
    }

I believe that users could accept this idiom, if open-methods were baked into
the language. I fear that, for a significant subset of those, this workaround
_on top of_ an emulation resorting on macros, it will be a little too much to
swallow.

But there's more...

Consider the AST example, using std::unique_ptr to manage the child nodes (or
shared_ptr, it doesn't matter):

    struct Node {
        virtual ~Node() {}
    };

    struct Plus : Node {
        Plus(std::unique_ptr<Node> left, std::unique_ptr<Node> right)
            : left(std::move(left)), right(std::move(right)) {}

        std::unique_ptr<Node> left, right;
    };

    auto value(virtual Node&) -> int;

    auto value(virtual Plus&) -> int {
        return value(left) + value(right);
    }

Perfectly reasonable. In OpenMethod, if I did not support virtual_ptr<>s on
smart pointers, it would have to look like:

    struct Plus : Node {
        Plus(std::unique_ptr<Node> left, std::unique_ptr<Node> right)
            : left(std::move(left)), right(std::move(right)),
              left_vptr(left), right_vptr(right)
             {}

        // for use with "final":
        Plus(
            std::unique_ptr<Node> left, virtual_ptr<Node> left_vptr,
            std::unique_ptr<Node> right, virtual_ptr<Node> right_vptr)
            : left(std::move(left)), right(std::move(right)),
              left_vptr(left_vptr), right_vptr(left_vptr)
             {}

        std::unique_ptr<Node> left, right;
        boost::openmethod::virtual_ptr<Node> left_vptr, right_vptr;
    };

    auto value(virtual Node&) -> int;

    auto value(virtual Plus&) -> int {
        return value(left_vptr) + value(right_vptr);
    }

I think that, at this point, some users would give up on virtual_ptr, and use a
virtual_<Node&> instead. But a call via virtual_vptr is three instructions only;
via a reference, nine.

Finally, "Design and evaluation of C++ open multi-methods", 5.3. Smart pointers
states:

> Defining open-methods directly on smart pointers is not possible. In the
> following example, (1) yields an error, as ptr1 is neither a reference nor a
> pointer type. The declaration of (2) is an error, because shared_ptr is not a
> polymorphic object (it does not define any virtual function). Even when
> shared_ptr were polymorphic, the open-method declaration would be meaningless.
> A shared_ptr<B> would not be in an inheritance relationship to shared_ptr<A>,
> thus the compiler would not recognize foo(virtual shared_ptr<B>&) as an
> overrider.
>
> void foo(virtual shared_ptr<A> ptr1); // (1) error
> void foo(virtual shared_ptr<A>& ptr2); // (2) error

I don't buy this. How is it different from plain pointers? A Cat* is not in an
inheritance relationship to Animal* either: Cat and Animal are, thus there is a
standard _conversion_ from one to the other.

My intuition is that virtual smart pointer arguments must be forbidden because
it would require the compiler to be aware of the standard 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.

To me too. YOMM2 doesn't do that. I am definitely bringing back that behavior as
a facet. Heck, I am tempted to make it the default again, and make the N2216
resolution an opt-in.

I am not hugely convinced with using the return type as a tie-breaker either.
For starters, it's one of those rules that apply only in certain cases
(covariant return types). I think it is more coherent, and easier to understand,
if we use exactly the same rules as overload resolution.

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

Maybe with reflection and generation in a few years. If the compilers are
capable enough.

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.

I didn't want to pile up on concepts right away. I thought I'd teach by example:
Look, this is just a virtual function! moved out of a class. Then dive in
more...But the approach doesn't seem to work.

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

Alas! After changing virtual_ptr to have pointer semantics, I forgot to remove
this bit of doc.

> * “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.

For example:

Node   Node
  |     |
  B     C
   \   /
     A

* 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>)).

I tried since you last suggested this. Couldn't make it work. The macro
generates something like:

auto poke(std::ostream& a, virtual_ptr<Animal> b) -> void {
    method<poke_method(std::ostream& a, virtual_ptr<Animal> b)>::fn(a, b);
}

There is no way to peel off the `void` from the signature with the syntax you
suggest.

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

This existed in the past, but it was lost when YOMM2 became capable of dealing
with non-polymorphic classes in some situations. I reinstated it.

> * 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 desirable.

I am considering separating them completely. As of now basing one on the other
is too error-prone.

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

Looks like a copy-paste mishap. Thanks for catching it.

> * “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”.

Maybe I am going to avoid defining it, e.g.:

    #ifdef BOOST_OPENMETHOD_DEFAULT_POLICY
    template<class Class, class Policy = BOOST_OPENMETHOD_DEFAULT_POLICY>
    #else
    template<class Class, class Policy = default_policy>
    #endif

* 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*/;

Yep.

> * “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?)

Windows, I am not sure. On the Mac? I don't have one. You may need to export or
import symbols via config files. That is not my concern, those are OS related
issues. If your dynamic linker works well with templates, it will work well
with OpenMethod.

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

It was a nightmare to document, and it looks like I did a poor job of it. The
set of facets is closed, because the dispatcher and the compiler use them in `if
constexpr` blocks etc. If you add your own facet, the library won't even look at
it. The set of facet _implementations_ is opened, and designed with the goal
that you can add your own.

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

They are needed to:
- write facet implementations
- implement intrusive vptrs (with_vptr)
- use the core API

Everything else is in `detail`.

Clearly the library has a "basic" interface and an advanced one. I should do a
better job at separating them in the doc.

> * 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)…)

The macro generates a `poke` function that forwardes to
`method<poke_boost_openmethod(...), void>::fn`.

By the way, creating such a forwarder is not easy at all. YOMM2 uses Boost.PP
looping macros to process the arguments. And you find OM's macro complicated?
;-) These macros are much simpler, and they can deal with types that contain
parentheses, without requiring BOOST_IDENTITY_TYPE.

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

I do:

> static poke::override<poke_cat> override_poke_cat;
> ...
> In C++26, we will be able to use _ instead of inventing a one-time-use
> identifier. In the meantime, OpenMethod provides a small convenience macro:

Core API exists primarily to make it possible to mix open-methods and templates;
secondarily to placate people completely allergic to macros ;-)

> * 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>>?

They're the same. virtual_shared_ptr is a templatized typedefs.

> * 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 think I see your point, but there are efficient alternatives to virtual_ptr:
- with_vptr, i.e. intrusive pointer to v-table
- you can take over vptr acquisition by providing a `boost_openmethod_vptr`
  function
- extern_vptr facets; YOMM2 has an example with one vptr per page of objects

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

It expands to the specialization of the class template that contains all the
overriders of the same name in the current namespace.

Actually, you are the godfather of this macro ;-) After your remark about the
arguments having a different look in different constructs, and after failing to
find a way to use a function type in the macros, instead of passing the return
type as a separate argument, I introduced the macro to provide an uniform look.

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

https://jll63.github.io/Boost.OpenMethod/#BOOST_OPENMETHOD

> * 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 knew this would be controversial.

You don't have to use it. You can just say:

    #include <boost/openmethod/core.hpp>
    #include <boost/openmethod/macros.hpp>

I can also document that macros.hpp includes core.hpp, so you can confidently
just say:

    #include <boost/openmethod/macros.hpp>

If it is still a big issue, then I can do it like in YOMM2:

    #include <boost/openmethod/keywords.hpp>

...so it is not as prominent as <boost/openmethod.hpp>

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

It is mentioned at the end of
https://jll63.github.io/Boost.OpenMethod/#tutorials_headers_and_namespaces

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

Probably it should go in the Custom RTTI tutorial.

J-L