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

From: Jean-Louis Leroy (jl_at_[hidden])
Date: 2025-05-09 02:13:11

Hi Steven,

Thank you for the review.

> virtual_ptr should not be put in the global namespace.

OK OK I surrender ;-)

> "That is because the overrider containers exist in both the canides and
> felides" The code has "canines" and "felines"

Six years of Latin in high school...

> BOOST_OPENMETHOD_OVERRIDER:
>    I think it would be better to make this macro expand to something
>    callable, i.e., add ::fn. That way the name fn can be an
>    implementation detail.

For consistency with BOOST_OPENMETHOD_OVERRIDERS. `fn` is used in other
places. OpenMethod makes more internals public for interoperation between the
macro and core interfaces.

I still have to try your (and Joaquin's) suggestion of replacing the comma
before the return type with an arrow in macros like BOOST_OPENMETHOD. If it
works (very likely), BOOST_OPENMETHOD_OVERRIDER loses its raison d'être and I
will probably remove it.

>    Why does BOOST_OPENMETHOD_OVERRIDER need the return type?

N2216 lookup. Considering return types as tie-breakers to resolve some
ambiguities. Which I am demoting to an opt-in.

> BOOST_OPENMETHOD_OVERRIDERS:
>    The documentation of friendship claims that this can be used to grant
>    friendship to all overriders. Is this really all overriders, or is it
>    only overriders in the same namespace?

In the same namespace. Doc fix.

> "Be aware, though, that the overriders of any method called poke -
> with any signature"
>   Does this imply that we can define multiple open
>   methods with the same name, but different argument types?

Yes. See
https://github.com/jll63/Boost.OpenMethod/blob/master/examples/matrix.cpp

> Switching the default policy based on NDEBUG is dangerous. It's
> not uncommon to link code build with NDEBUG to code built without it.

Hmmm yes I see that. I am completely separating the release and debug policies
in the new design of the policy system. default_policy is a typedef, and, if I
understand properly, having a typedef aliasing to different types is not an ODR
violation in itself, but it makes it easier to create one.

Do you have a better suggestion?

> I don't understand the name vectored_error_handler. How is it vectored?

I guess it's boomer-speak... We used to call indirect function calls "vectored".
Do you have a better name?

> I'm confused about how error handling works. The docs say "When an error
> is encountered, the program is terminated by a call to abort. If the
> policy contains an error_handler facet, it provides an error member
> function (or overloaded functions) to be called with an object
> identifying the error." I'm guessing this means that abort is called
> if there is no error handler or if the error handler returns normally.

Yes indeed. I have problems phrasing that, perhaps related to English not being
my first language.

> I think the facet interface could be simplified.

Following the discussion with Ruben, I came up with a much simpler, safer
design. CRTP and `fork` are gone.

> In many places the if constexpr(has_facet<>) logic could be simplified
> by using a no-op implementation of the facets instead of letting facets
> be missing.

Yeah I actually do that in a couple of places. Like finalize() inherited from a
base class by all policies. I sort of felt uneasy with resorting to shadowing.

> detail/static_list.hpp:
> - Most of the iterator functions can be constexpr

For what benefit? It is an implementation detail specifically designed to be
used by static ctors...

> detail/trace.hpp:
> - It's a bit weird to have the indent of 4 split into 2*2.
>    It would make more sense for indentation_level to indicate either
>    the exact number of spaces or the depth.

Oh right...but you didn't see it, it's in `namespace detail` ;-)

> policies/basic_policy.hpp:
> - fork_facet is quite dangerous. It assumes that if a facet
>    is a template, the first template parameter is the policy.
>    This is potentially surprising and I don't see where
>    it's documented.

It's better than documented now, it's GONE.

> policies/basic_error_output.hpp:
> - Why use virtual inheritance?

No good reason. Not in the new design.

> policies/vptr_vector.hpp:
> 32: Does using namespace policies do anything?

It imports the names in the current scope? ;-) I do that in the body of some
functions.

> compiler.hpp:
> - class_::is_base_of: Why use std::find on an unordered_set?

I think those sets used to be vectors. Maybe they should be again.

> 266: in compiler::static_type
> [...]
>       ...seems to be unused.

Yes, I will remove it.

> 343: if (*method.vp_end == 0) {
>    Since this is inside the loop, won't it only resolve the first type?
>    Actually, I'm really confused about the whole loop. Do we
>    want to process the overriders for each virtual parameter?

That code has nothing to do with resolving overriders. It is for deferred RTTI.
For interfacing with custom RTTI schemes that also use static ctors. If the
deferred_static_rtti is present, I store a pointer to a function that returns a
type_id, instead of the type_id itself. initialize() calls the function and then
replaces the function pointer with the type_id. Since initialize() can be called
multiple times, I add a flag just after method.vp_end.

J-L