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

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2025-05-10 19:12:27

AMDG

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

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.

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 clearly doesn't strictly mean a C++ polymorphic
   type, because std::is_polymorphic would always be
   right, then, but I don't see a definition of what
   it does mean.

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

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.

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 tried adding an rtti facet, but it didn't help.

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.
- I'm particularly concerned about the way reinterpret_cast
   leaks into user code in the rtti policy's type_name.

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 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. If an unimplemented method aborts, that implies
that it is considered a programming error. In this case I want to check 
for completeness in initialization and fail if anything is missing. 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.

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. Failing on initialization would also be acceptable. I'm not 
particularly fond of using the covariant return type to choose. It's 
unlikely to be less surprising than just picking one at random, since
that's not how C++ normally does overload resolution. What about 
prioritizing the leftmost parameter? That's simple and predictable.

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

- std::any can use .type()
- boost::type_erasure::any can either use std::type_info or
   it can be configured to store a vptr.

Both provide any_cast which can be used to cast the value.

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.

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

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

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.

Consider the following example:

struct Base {
   virtual ~Base() = default;
};

struct D1 : Base {};
struct D2 : D1 {};
struct D3 : D2 {};
struct D4 : D3 {};
struct D5 : D4 {};

BOOST_OPENMETHOD_CLASSES(Base, D1, D2, D3)
BOOST_OPENMETHOD_CLASSES(D2, D3)
BOOST_OPENMETHOD_CLASSES(D3, D4)
BOOST_OPENMETHOD_CLASSES(D4, D5, D3)
...
Inheritance lattice:
     D3
         bases:      (D2)
         derived:    (D4, D5)
         covariant: (D4, D5, D3)

compiler.hpp:554: typo "unkown"

compiler.hpp:735:
     if (!cls.used_by_vp.empty()) {
         for (const auto& mp : cls.used_by_vp) {
    The if seems redundant.

The algorithm for assign_lattice_slots looks pretty inefficient to me. 
The worst case is O(slots^2 * classes^2). It can also generate results 
that are obviously silly (at least to a human) pretty easily. I know 
optimal slot allocation is NP-hard, but I think it can be tweaked in a 
few ways.

Considering the optimization of not storing leading unused slots, 
filling the lowest available slot is a suboptimal strategy. We can pack 
it tighter by finding the ranges from bases (and derived) and filling in 
holes.

Notice that the algorithm's correctness does not depend on visiting the 
nodes in any particular order. If we're allocating from 0, it's probably 
most effective to visit types bases first instead of depth first. If 
we're trying to allocate contiguous ranges, we can move both up and down 
from types that are already visited. I believe that in the absence of 
virtual inheritance, this is guaranteed to assign slots contiguously 
without leaving any holes.

After assigning a slot, we only need to propagate a single bit across 
the hierarchy. We don't need to merge all the bit vectors.

Finally, we could pull reserved slots up from transitive derived before 
allocating slots instead of pushing them up to transitive bases of 
transitive derived after slot allocation. This would reduce the nesting 
depth of the loops.

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.

In Christ,
Steven Watanabe