
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