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

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2025-05-07 23:50:56

AMDG

Here's an incomplete review. There definitely isn't enough
time left for me to finish it today.

Review revision: ea7dbe86b511797f0281ef07b3ada645fe569328 (master)

virtual_ptr should not be put in the global namespace.

s/Chhetah/Cheetah/

"It can take be used with any class name..." - remove "take"

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

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.

   Why does BOOST_OPENMETHOD_OVERRIDER need the return type?

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?

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

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

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

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.

I think the facet interface could be simplified.
- Why do we need to have the implementation inherit from the facet?
   Alternately, if we have this inheritance, we should be able to
   specify just the implementation in replace<>.
- I'm a bit confused about the difference between
   release::add<runtime_checks>, and
   fork<default_policy>::replace<error_handler, throw_if_not_implemented>
   I'd rather have a single way to create a new policy, and the
   former seems simpler.

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.

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

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.

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.

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

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

compiler.hpp:
- class_::is_base_of: Why use std::find on an unordered_set?
266: in compiler::static_type
         if constexpr (std::is_base_of_v<
              policies::deferred_static_rtti, policies::rtti>) {
      Did you mean to check the rtti facet of Policy?
      ...seems to be unused.

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?

In Christ,
Steven Watanabe