El 27/04/2025 a las 15:15, Дмитрий Архипов via Boost escribió:
Dear Boost community. The peer review of the proposed Boost.OpenMethod will start on 28th of April and continue until May 7th.
This is my review of the Boost.OpenMethod proposal. Thanks to Jean-Louis for his work and to Dmitry for managing! As a guide to this review, I'm answering the points proposed by the review manager succintly and then a full review section is given at the end with much more detail (in no particular order). * What is your evaluation of the design? I find it adequate and well thought out, maybe leaning on overcomplexity for the sake of covering as much as possible (below). * What is your evaluation of the implementation? I didn't look closely, but what little I saw looked to me robust. Code looks clean, too. * What is your evaluation of the documentation? Quite extensive and probably covers anything that needs be covered, but I have qualms about it (below). * What is your evaluation of the potential usefulness of the library? It is very useful if you need open methods, obviously. Personally I haven't delved much into the world of virtual functions (I'm more of a template programmer), but the fact that the predecessor of this library (YOMM2) has real users is enough to convince me that the proposal fills a need. * Did you try to use the library? With what compiler? Did you have any problems? Yes, to get my head around it by writing small snippets. VS 2022. No problems detected. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent 11 hours playing with the library and reading and re-reading the docs. * Are you knowledgeable about the problems tackled by the library? I'm familiar with open methods since I read about them in Alexandrescu's 2001 "Modern C++ Design" (where they were called multimethods). I think I'm sufficiently familiar with virtual function machinery to understand the technical challenges met by this proposal and the solutions adopted. * Do you think the library should be accepted as a Boost library? Yes, I vote to ACCEPT Boost.OpenMethod. I have some observations/reservations about it, some of them more important than others. To be clear, I'm not conditioning my vote on these observations being addressed, since I know the author will ponder them carefully and respond in the most appropriate way --which may not be the way I suggest. FULL REVIEW Main points: * The docs are quite hard to read (to me, at least). Mainly, I miss clear introductions and explanations of the many concepts involved before using them in the tutorial. A (probably incomplete) list of concepts that I had to wrap my head around is: + virtual_ptr, double role as a fat reference and as a virtual argument marker + virtual_ + compiler + guide function + policies and facets * Policies, in particular, are introduced in a way that feels to me haphazard and incomplete. I never can be sure if I have all the policy-related information correctly and exhaustively. * I’d encourage the author to try and remove from the docs (i.e. not disclose publicly) as many utilities as possible – I have the feeling some components are described that don’t really belong in the public API, though I may be wrong on this one. * I think virtual_ptr is a misnomer and the entity should be renamed to virtual_ref or something similar. * I challenge the need to have virtual_shared_ptr, virtual_unique_ptr and the like. In my opinion, the proposal need not conflate virtual arguments with object lifetimes, the latter being orthogonal to the purpose of the library. * The decision to call an arbitrary override when there’s a tie-in looks erroneous to me, and it may at least be controlled by some policy facet. * The library relies very heavily on macros, and the macro-free alternative does not seem too practicable. I guess this can’t be helped, but it’d be nice if some thought were given to how to alleviate this overdependence on macros. Elaboration and full list of observations: * virtual_ptr. I have a number of problems with this class: + The documentation starts using it right away without telling the user what it is about. At first, I thought it was merely a syntactic marker to indicate wher a virtual argument happens, which is the case, but not the whole story. + Its semantics are not those of a pointer, but behave more like a reference. This is explicitly acknowledged later in the docs, on the grounds that virtual_ref, which is a more apt name, is reserved for potential evolutions of the C++ language to include overloading of the dot operator. I think it is extremely unlikely that this C++ feature will ever be realized. + As it happens, an open method works equally well if a reference to a class is passed instead of a virtual_ptr (this is apparent as soon as one realizes that virtual_ptr can be constructed from a plain reference, but I didn’t read that far into the reference). In a private communication with the author, he told me this is indeed the case but virtual_ptr is more efficient because it stores an extra direct pointer to the associated virtual table, in a manner not dissimilar to what Rust’s fat pointers do (again, this is mentioned later in the reference). I was led astray by the implicit assumption that virtual_ptr was a sort of pointer, which is not. + What’s the point if having virtual_ptr() and virtual_ptr(nullptr_t) constructors if virtual_ptr behaves as a reference? Same with the move constructor, what’s the point of emptying the source virtual_ptr? + virtual_ptr<const C> is constructible from virtual_ptr<C> but not the other way around, as it should be. This is not readily apparent from reading the reference, though. + All in all, I strongly recommend that virtual_ptr be renamed to virtual_ref, and that an early explanation in the docs is given for its presence as opposed to using plain references. operator -> can be retained because it provides a terse syntax to access the underlying object, but consider providing as well some value() member function and/or a conversion operator to the underlying object. * “Multiple and virtual inheritance are supported, with the exception of repeated inheritance”: I don’t know what repeated inheritance means –I may make an educated guess, but I don’t think this is standard terminology. * Instead of BOOST_OPENMETHOD(poke, (std::ostream&, virtual_ptr<Animal>), void), consider the possibility of using BOOST_OPENMETHOD(poke, void(std::ostream&, virtual_ptr<Animal>)). * When virtual_ptr is used, classes are required to be polymorphic (i.e. classes with a virtual table), which is perfectly ok. But if I fail to declare any virtual function in my class hierarchy, simple snippets of code still compile and produce spurious results. It would be good if this could be caught and signaled by a compile-time error. * policies:debug and policies:release: I understand that these do not mix together? I mean, if a DLL is compiled in debug mode, its exported open methods won’t be taken by an executable compiled in release mode, right? Not sure if this is expected and/or desireable. * “If there is still no unique best overrider, one of the best overriders is chosen arbitrarily.” I’m not sure this is the best default option, probably a run-time error is more appropriate here, maybe controlled by some facet of the policy. * In the description of the release policy, it’s stated that, for instance, the facet extern_vtpr is implemented with vptr_vector and vtpr_map. This doesn’t make much sense to me, as a policy can’t possibly provide more than one implementation for a given facet. Looking at the source code I learnt that it’s vptr_vector that’s used for extern_vptr in release. * “If BOOST_OPENMETHOD_DEFAULT_POLICY is defined before including this header, its value is used as the default value for the Policy template parameter throughout the code. Otherwise, boost::openmethod::default_policy is used.” I think it woud be more correct to say “Otherwise, BOOST_OPENMETHOD_DEFAULT_POLICY is defined to boost::openmethod::default_policy”. * In the reference, initialize is declared as: template<class Policy = BOOST_OPENMETHOD_DEFAULT_POLICY> auto compiler<Policy>::initialize() -> /*unspecified*/; I understand that this should be template<class Policy = BOOST_OPENMETHOD_DEFAULT_POLICY> auto initialize() -> /*unspecified*/; * “OpenMethod supports dynamic loading on operating systems that are capable of handling C++ templates correctly during dynamic link.” I don’t understand what this means, and what operating systems are those (All the usual ones? Or is it a rare feature?) * I miss a complete specification of what facets and other pieces of info a policy can/must have, what happens when some particular facet/piece of info is not explicitly provided, and what facets/pieces of info can be user-defined instead of simply chosen from the available catalog of options in the library. * The reference includes lots of small classes such as type_id, vptr_type, etc. but it’s not clear to me if these are implementation details or things for which a user can provide alternative implementations within a policy. If the former, I’d remove them from the reference. * The section “Core API” explains how to implement the “poke” open method without utility macros. But then the example uses “poke::fn(…)” rather than plain “poke(…)”. What additional step is needed to achieve “poke(…)” syntax as macro-supported examples accomplish? (Peeking into the code, I see that BOOST_OPEN_METHOD includes the definition of a free-standing forwarding function inline auto NAME(ForwarderParameters&&... args)…) * Core API: the example uses BOOST_OPENMETHOD_REGISTER, it would be nice to show how to get rid of all macros, including this one. * A virtual argument in an open method can be defined with virtual_ptr<C> or virtual_<C>. I understand why these two exist--the former is more efficient. But then we also have virtual_shared_ptr and virtual_unique_ptr: what’s the advantage of using virtual_shared_ptr<C> instead of std::shared_ptr<virtual_ptr<C>>? This seems to add some clutter to the lib for no real benefit, plus it conflates the concepts of “virtual slot” and “object lifetime”, which in my mind are completely orthogonal, the latter not really belonging in this library, much like classical virtual functions are not concerned with object lifetime either. FWIW, this extension is not part of N2216. * Elaborating on the previous point, a cleaner approach (IMHO) would be to mark virtual slots with virtual_<C> exclusively, and then let this interoperate smoothly with virtual_ptr <C> (or virtual_ref if the name is finally changed). That is, virtual_ is concerned with the signature of open methods only, and virtual_ptr is concerned with run-time optimization of reference passing, only. * I understand that BOOST_OPENMETHOD_DECLARE_OVERRIDER and BOOST_OPENMETHOD_DEFINE_OVERRIDER separate the declaration and definition that are covered together in BOOST_OPENMETHOD_OVERRIDE. But what does BOOST_OPENMETHOD_OVERRIDER do? The explanation in the reference is quite opaque to me. * The docs talk about so-called guide functions, but I can’t seem to find this concept defined anywhere. * Regarding <boost/openmethod.hpp>, it’s stated that it “[a]lso imports boost::openmethod::virtual_ptr in the global namespace. This is usually regarded as bad practice. The rationale is that OpenMethod emulates a language feature, and virtual_ptr is equivalent to keyword, similar to virtual.” I agree this is bad practice, I’d recommend against it. * I can define and override an open method inside a namespace (say open method run my_namespace), so that’s ok. But I can’t override outside the namespace with BOOST_OPENMETHOD_OVERRIDE(my_namespace::run,…). At least, this should be mentioned as a limitation of the macro-based interface. * I know that an internal type_hashed is required because of previous communications with the author around this very point, but I have the feeling this piece of the lib is not adequately explained in the docs and a casual reader may be confused by it. Joaquin M Lopez Munoz