On Sun, 27 Apr 2025 at 15:15, Дмитрий Архипов via Boost <boost@lists.boost.org> wrote:
Dear Boost community. The peer review of the proposed Boost.OpenMethod will start on 28th of April and continue until May 7th. OpenMethods implements open methods in C++. Those are "virtual functions" defined outside of classes. They allow avoiding god classes, and visitors and provide a solution to the Expression Problem, and the banana-gorilla-jungle problem. They also support multiple dispatch. This library implements most of Stroustrup's multimethods proposal, with some new features, like customization points and inter-operability with smart pointers. And despite all that open-method calls are fast - on par with native virtual functions.
Hi all, This is my formal review of the proposed Boost.OpenMethod. Thanks Jean-Luis for proposing the library, and Dmitry for managing the review. I'll follow the points suggested by the review manager. I've numbered my comments to make replying easier.
* What is your evaluation of the design?
1. The library achieves the goal of emulating open methods without language support, which is quite impressive. The amount of macros in the interface makes it a little bit dirty, but I guess that's unavoidable when trying to emulate a language feature. I have to say I'm not a big fan of open methods because they move a lot of checks that usually happen at compile time to runtime. But I acknowledge that this pays off with extra flexibility. Taking into account the popularity of yomm2, the predecessor of the proposed library, it looks like open methods fit a real need. 2. That said, I think that policies and facets would benefit from some re-designing. a. Aside from rtti, the other facets have no formal specification on their requirements. This means that there is no formal API for these types. This makes it prone to breaking users in subsequent releases, and may block evolution by fearing breaking users. b. The design heavily relies on static members in policies and facets, which are effectively global variables. It then adds a set of tools and techniques (CRTP, basic_policy::add, basic_policy::fork) to deal with the problems caused by the former. I think that these problems can be avoided altogether by redesigning the system. c. Policies have two functions: stating how to do things (i.e. stating that vptrs should be stored in a vector) and acting as a type registry (i.e. allocating the concrete vector for vptrs as a static member). I think that this breaks separation of concerns. Policies should only state how to do things, and a separate "type registry" entity should be in charge of creating the required static variables required to register types. I've already suggested a possible design for this in a previous message (https://lists.boost.org/Archives/boost/2025/05/259451.php), so I won't repeat it here. If the library ends up being accepted, I can make a PR with the proposed changes. 3. The boost_openmethod_vptr customization point does not seem to play well with policy scoping. Given a type to be inspected, T, the signature is: std::uintptr_t boost_openmethod_vptr(const T&); There is no way to know which policy T was registered with. This function normally uses Policy::static_vptr, so this is a problem. Ideally, it should be redesigned to cope well with policy scoping. 4. Compile-time errors are difficult to interpret. For example, at one point, I wrote: BOOST_OPENMETHOD(print, (virtual_<base_node>), void); This is a mistake because the argument to virtual_ should be a reference type. This is a snippet of the compiler error I got: "error: no member named 'peek' in 'boost::openmethod::detail::parameter_traits<boost::openmethod::virtual_<base_node>, custom_policy>'" I had to look at the implementation to see what was the problem, and it took me some time to figure it out. I think that this problem can be alleviated by using C++20 concepts in places like virtual_. Since the library's minimum standard is C++17, you'll need some macro machinery to make this work. Other libraries like Asio do it with good results. 5. virtual_<T&>, with T& not being polymorphic, compiles. In the program I was writing, this was a programming error - I had forgotten a virtual destructor. This behavior seems to exist to support "programs that call methods solely via virtual_ptrs created with the "final" construct", as stated in minimal_rtti's docs. When would I want to do this? Does this not defeat the purpose of using open methods themselves? 6. The <boost/openmethod.hpp> file is unusual. It does not include all the headers in the library, and it pours definitions in the global namespace. I'd advise against doing this, because it's not what most users expect. My recommendation is to: a. Rename this file to <boost/openmethod/keywords.hpp> or any other name that you think it's helpful. b. Make <boost/openmethod.hpp> include all headers except for keywords.hpp. Another possibility would be to create a namespace boost::openmethod::keywords that contains using directives for virtual_, virtual_ptr and similar pseudo-keywords. The user can then consume these with a using namespace directive, similar to how custom literals are consumed. 7. boost::openmethod::type_id is a type alias for std::uintptr_t. I would try to avoid such an alias, since it doesn't add much and makes code more difficult to read. It might be misleading given the existence of the typeid operator. * What is your evaluation of the implementation? I haven't examined it in detail. Aside from my comments on static members, the pieces I looked into seemed good. CIs would benefit from some extra coverage - at the moment, only the latest compiler versions, with their default standard C++, seem to be checked. * What is your evaluation of the documentation? It contains enough information to cover the basic use cases, but it needs work. These are my main comments: 8. I think that the idea of splitting the docs into a "basic" and an "advanced" section, as proposed by the author before, is great and should happen. 9. I dislike the single-page architecture. I know that Boost.Unordered (https://www.boost.org/doc/libs/1_88_0/libs/unordered/doc/html/unordered/intr...) uses Antora to render asciidoc as multiple pages - maybe it's a technology that's worth to explore. 10. Facets need a formal API - a section that documents what members are required and which signatures should they have. 11. In general, some concepts seem to be used without being introduced first: a. Overrider. When I first read through the docs, I assumed it meant "the set of functions that implement an abstract method". When I reached the description of BOOST_OPENMETHOD_OVERRIDER, I saw that it refers to a concrete C++ struct with a concrete naming scheme and set of members. I think that this should be explained better, before BOOST_OPENMETHOD_OVERRIDER is introduced. b. Domain. By reading the code, it refers to a subset of the type registry. It's used in the reference (e.g. "debug extends release but it does not fork it. Both policies use the same domain"). c. "virtual_ptr's "final" constructs". Mentioned in the docs without defining what they are. 12. The custom RTTI example uses a virtual destructor. This is important because the default policy's is_polymorphic implementation uses std::is_polymorphic, but I don't think it's explained. I don't know if having a virtual destructor is common in custom RTTI scenarios, but I'm inclined to think that it's not, as AFAIK a virtual destructor adds standard RTTI information to a type. Clang's AST nodes don't have it. I'd advise to add an example on how to support a custom RTTI scheme without virtual destructors, as it wasn't trivial for me to implement it. More minor points at the end of this email. * What is your evaluation of the potential usefulness of the library? I don't have a use case for it, and I wouldn't use it in my projects, since I think it adds too much complexity for the benefit it brings. However, considering the traction that its predecessor, yomm2, has, I think it would be useful in Boost. It's a pity that no standardization is pursued. Being a language emulation library, it looks like eventual standardization should be a goal. But I understand that it might not be feasible at this point. * Did you try to use the library? With what compiler? Did you have any problems? I built one of the examples and played with it. I then wrote a toy example to check how an API like clang's AST could integrate with this library. I had some problems at first, due to my lack of understanding of the library design and the amount of checks that move to runtime. But I was successful in the end. With some documentation improvements, it'd be enough to be able to use the library without problems even in advanced use cases. I used clang 20, C++23, with CMake's Debug configuration, in an Ubuntu 24.04 system. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I've spent around 15h reading the documentation, building and playing with examples, reading parts of the source code, asking questions and writing this review. * Are you knowledgeable about the problems tackled by the library? I heard about open methods at the author's talk at using std::cpp 2024. I hadn't used them before this review. I have designed and used class hierarchies and OOP before. * Affiliation disclosure I am currently affiliated with the C++ Alliance. * Final verdict Although I'm not fully convinced, my recommendation is to ACCEPT this library into Boost, as I think Boost is better with than without it. I strongly recommend redesigning the facet/policy system, but I won't make it an acceptance condition because I don't know if what I propose is fully viable. * Minor documentation comments 13. Code in the dynamic loading section looks a little bit too C. In particular, I'd try to avoid strcat (vs. strncat), since it leads to bad programming practices that can end up in buffer overruns. 14. In the description of the RTTI policy, in the type_name function: "This function is optional; if it is not provided, "type_id(type)" is used.". I'd phrase it as "the numeric type_id is inserted into the stream, as per stream << type". I originally thought it was a typo and that typeid(type).name() was used. 15. I'd try to avoid the "bom" alias, in favor of "namespace openmethod = boost::openmethod;". 16. The std_rtti::type_index reference says that the return value is unspecified, and then it states that it returns a std::type_index. 17. Reference docs for use_classes mention Policy as an extra template argument, which is not what the actual code looks like. Instead, the passed arguments are inspected, and if one is a policy, it is used as the class registry. This should be noted in the docs. It is also not clear what happens if several policies are specified in a single use_classes instantiation. Regards, Ruben.