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

From: Jean-Louis Leroy (jl_at_[hidden])
Date: 2025-05-11 00:33:33

> Sorry for the late feedback. I finally had enough time to get through most of
> the implementation.

You have sharp eyes :)

> boost_openmethod_vptr returns a result that is policy dependent. Should it be
> possible to overload it based on the policy? At the very least, it should be
> possible to verify that the policy matches.

Ruben pointed that out too.

boost_openmethod_vptr() is a recent invention. YOMM2 looks for a *public*
boost_openmethod_vptr *member* in the object. I changed it to a function because
it allows a base class to use whatever name it wants for the member; and, more
importantly, the said member can be private, and the base can define
boost_openmethod_vptr() as an inline friend.

I did not anticipate that one would have the idea of calculating the *value* of
the vptr there. The way I use it is to retrieve the vptr set by with_vptr's
constructor.

I experimented with passing a Policy* as the first argument to the function
after Ruben's remarks. It's not hard to make it work, but it requires
boost_openmethod_vptr() to either specify the exact policy it expects, or be a
template if it wants to ignore it.

It also makes it possible to associate more than one vptr to an object. Why not?
As we say in French, who can do more of it can do less of it (qui peut le plus
peut le moins).

> Custom RTTI:
> - "is_polymorphic is used to check if a class is
>    polymorphic. This template is required."
>    What does it mean for a type to be polymorphic?

It means polymorphic from the point of view of the rtti facet. For std_rtti it
means having virtual function(s). For custom rtti, it could be deriving from a
base, or having certain members. I'll need to explain that in the doc.

> - It would be helpful for debugging if method dispatch
>    asserted that the policy was initialized. It took me
>    a while to figure out that I was still initializing
>    the default policy and not my custom policy.

Yes. I've been bitten by that too.
https://github.com/jll63/Boost.OpenMethod/issues/16

> I think virtual_ptr is doing too much. What would it take to make it
> possible to implement virtual_ptr without any special support in the
> core library?
> - boost_openmethod_vptr provides a way to get the vtable
> - The rtti policy and/or static_cast should allow downcasting
>    to the overrider parameter type.
> - The only thing that is clearly missing is the ability to
>    indicate that virtual_ptr is a virtual parameter.

In YOMM2, virtual_ptr is an afterthought. While preparing OpenMethod for review,
I struggled with explaining clearly and simply that `virtual_` should be used
only in the base-method, and *not* in the overriders. YOMM2 beginners often
struggled with that. Then I though, on top of that, virtual_ptr gets us dispatch
in three instructions instead of nine (for 1-methods). Shouldn't it be the
"golden path"? And virtual_ be demoted to an entry point into advanced stuff?

> I tried using shared_ptr as a parameter type like this:
>
> BOOST_OPENMETHOD_CLASSES(Base, Derived)
> BOOST_OPENMETHOD(foo, (virtual_<std::shared_ptr<Base>>), void)
> BOOST_OPENMETHOD_OVERRIDE(foo, (std::shared_ptr<Derived>), void) {
>    std::cout << "derived" << std::endl;
> }
>
> - The guide function doesn't work. I don't understand why,
>    since shared_ptr<Derived> is convertible to shared_ptr<Base>.

I am surprised. I have a unit test for that (cast_args_shared_ptr_by_value). You
didn't attach the example, here is what I tried:

> #include <boost/openmethod.hpp>
> #include <boost/openmethod/compiler.hpp>
> #include <boost/openmethod/shared_ptr.hpp>
>
> #include <iostream>
>
> class Base {
>   public:
>     virtual ~Base() {
>     }
> };
>
> class Derived : public Base {};
>
> BOOST_OPENMETHOD_CLASSES(Base, Derived);
>
> BOOST_OPENMETHOD(
>     foo, (boost::openmethod::virtual_<std::shared_ptr<Base>>), void);
>
> BOOST_OPENMETHOD_OVERRIDE(foo, (std::shared_ptr<Derived> dog), void) {
>     std::cout << "Derived\n";
> }
>
> auto main() -> int {
>     boost::openmethod::initialize();
>     auto obj = std::make_shared<Derived>();
>     foo(obj);
>
>     return 0;
> }

https://godbolt.org/z/s7EhM1Yn4

By the way, do you have an opinion on allowing smart pointers as virtual
parameters?

> The (lack of) type safety is quite concerning.
> - I don't think the standard guarantees that a function
>    pointer can round-trip through void*. IIRC, an
>    arbitrary function pointer type like void(*)()
>    is guaranteed to work.

Adding an issue for this.

V-tables can contain a mixture of function pointers, data pointers and indexes.
Can a uintptr_t hold a function pointer?

cppreference.com says:

> uintptr_t (optional) unsigned integer type capable of holding a pointer to void

Optional? Does it mean that an implementation is not obliged to provide it?

And it looks like it can hold a data pointer, it doesn't say "any pointer".

At some point YOMM2 used a union for v-table entries, I may have to go back to
that.

> - I'm particularly concerned about the way reinterpret_cast
>    leaks into user code in the rtti policy's type_name.

That's only for users creating their own rtti facet. I consider that advanced
usage.

What a facet's type_name() gets is what the same facet returned from
static_type() and dynamic_type(), so the cast is safe (as long as the data fits
in a type_id).

I don't think think it's reasonable to templatize the entire library on type_id.

> throw_error_handler directly throws the error type as an exception, but
> openmethod_error does not inherit from std::exception. Even though this
> is perfectly legal, dealing with such exception can be rather
> painful.

I am not exception-phobic. Quite the opposite. But doing this would mean that
the error classes would be different whether or not exceptions are enable. I
tried to avoid ifdefs as much as I could...

> I don't think that the default error behavior for unimplemented
> overrides is good. I'm okay with an abort when initialization fails. An
> error that happens reliably on program startup is not much worse than a
> compile error.

Agreed. You can check the `report` in initalize()'s return value for that. And,
some day, I will provide a pre-linker or at least a linter.

> If a missing override isn't a bug, then it needs to be recoverable, or at
> least allow clean shutdown, which means throwing an exception.

Eh! The unit tests do that.

So you would prefer to make the exception-throwing handler the default, and let
people who cannot or will not accept exceptions tune the policy?

> Unlike some others, I'm actually okay with choosing some overrider when
> it's ambiguous. It's not a great solution, but it's still better than
> aborting.

It will be a choice. Now the question is to pick the best default. I am leaning
towards the YOMM2 way: no more and no less than (static) overload resolution.

> I'm not particularly fond of using the covariant return type to choose.

So you are halfway to N2216 :-D I can always split the "pick random" and "use
covariant return type" into two independent choices.

> What about prioritizing the leftmost parameter? That's simple and predictable.

IIRC that's what CLOS does. Dylan too probably. And nothing in C++ works that
way.

> I'd like to be able to use std::any or boost::type_erasure::any in open
> methods.

I considered std::any virtual parameters before. They seem to be within close
reach. Unlike std::variant, they're open, so it makes sense. I have a design, in
which you have to register the possible types, just like registering classes.

> I defined a custom rtti facet. It doesn't work, and it appears that the
> reason is that int does not inherit from any, which causes it to not be
> considered by assign_tree_slots. So, I used class_declaration_aux
> directly to forcibly make any a base of int, and then it seems to work.

Where there's a will... :-D

> The simplest way to solve this is to allow a policy to customize
> is_base_of and is_abstact.

This is an interesting research path...abstract sub-categorization away from
inheritance...I think that Clojure does something like that.

A few times people requested value-based dispatch for YOMM2. Like in CLOS. I
have a design for this. But then the latest requester wnats dispatch on *ranges*
of values. I wonder if there is a general way of allowing such extensions, that
is also practical.

Having open-methods in a library rather than in the language is sort of
liberating...

> I've attached files that show the result of my experiments (test_te.cpp
> and test_any.cpp).

I'll look at them closely. I haven't yet as I am writing this.

> compiler.hpp:
> 405-478: Calculating transitive_bases and direct_bases.
>    I don't think this works. The documentation claims
>    that the only restriction is that every type must
>    include its direct base classes, but the loop at 405
>    does not compute the transitive closure. This can
>    cause direct_bases to contain indirect bases as well.

So I thought I had found a smart trick for collecting the transitive closure at
compile time, but you are right, in your example, D5 is not aware that D3 is a
base of D4.

That shouldn't take me long to fix.

> It can also generate results that are obviously silly (at least to a human)
> pretty easily.

Do you mean silly and incorrect, or just silly?

> I know optimal slot allocation is NP-hard, but I think it can be tweaked in a
> few ways.

I am preserving the following paragraphs in
https://github.com/jll63/Boost.OpenMethod/issues/19 I will get back to it, but
it is not a priority at the moment. Thanks a lot for the input though.

> I believe that in the absence of virtual inheritance, this is guaranteed to
> assign slots contiguously without leaving any holes.

I am puzzled by this remark, because YOMM2 and OpenMethod are a bit myopic
regarding virtual and repeated inheritance.

At the bottom of it, the library suffers from being able to have only one
v-table per object. That is why repeated inheritance is not supported. And that
is the reason for the contortions about slot allocation.

> compiler.hpp:1147:  *cls.static_vptr = gv_iter - cls.first_slot;
> This is quite scary. It's undefined behavior if it goes before the
> beginning of the vector. I think I can trigger undefined behavior with
> the following hierarchy
>
>      C
>     /
>    A   B
>     \ /
>      D
>
> (see test_vptr_oob.cpp)
> ...
>   Initializing v-tables at 7c59751e1240
>     0 7c59751e1240 vtbl for A slots 2-3
> ...
> Notice that the vtable for A is placed first in the vector, but
> first_slot is 2.

Oh, right. I doubt any real program will crash on this, but you are right, this
is UB, and it can be fixed, so let's fix it. It is probably just a matter of
ordering the v-tables from lowest first_slot up, and perhaps from highest size
up if first_slot is the same.

I guess that you will frown at the idea of using the same trick to avoid storing
entries in the perfect hash table that come before the minimum value in the hash
function's image.

J-L