[OpenMethod] Andrzej's review
Hi Everyone, This is a review of the proposed Boost.OpenMethod library. First of all, I want to thank Jean-Louis for writing and sharing this library, for undergoing the Boost Review process, and for reviving this interesting and important subject. My recommendation is to *reject* the Boost.OpenMethod library at this time. My decision is based on the design issues. I note that rejection is not a permanent verdict, and there are known Boost libraries that were initially rejected, and then were improved and passed the second review. The library claims to solve two problems: the "expression problem" and the "banana-gorilla-jungle problem". In my opinion, the first problem is not a technical problem worth solving at all. It is more like a logical challenge: can you provide a framework extensible in two dimensions? Maybe I can, but so what? What real program or library benefits from this? The second problem is real, and although the docs do not present it adequately, during the discussion, and also via links to YOMM2 library, Jean-Louis provided a very compelling example of printing a Matrix which is from a OO hierarchy. I am personally not a fan of OO. I use very sparingly anything that has `virtual` on it. For instance, for heavy matrix computations, I would never think of using OO-hierarchies of matrices. It looks to me that OOP is limited in a language without a garbage collection, and any attempts at fixing it can only be limited. The problem is real. I would call it differently: "I want a runtime-polymorphic behavior, but I do not want to commit to all polymorphic functions up front". The library provides a solution for it. It is not necessarily a good solution, and it is for sure not the only solution to the problem. Two other possible solutions: 1. Use std::variant and variant visitors. For a non-trivial motivating example see https://akrzemi1.wordpress.com/2016/02/27/another-polymorphism/ 2. Use OO-style class hierarchies with pattern-matching on top: either wait for the C++ pattern matching, or use a library like Mach7 ( https://github.com/solodon4/Mach7) The docs in OpenMethod are missing the discussion on these alternatives. Especially alternative #2 seems to offer a superior solution, simpler, and with fewer gotchas. It is not clear to me what is the relation between Boost.OpenMethod and N2216. Is the goal to implement N2216? Or is it just mentioned for context, or as a loose inspiration? I hear the replies in the review discussions, "but this is what N2216 said", also the docs say that the library implements N2216 with extensions. But note that N2216 calls itself "a report" and the authors never claimed that they had a solution suitable for standardization. It was an exploration. But the solution in N2216 has one important property: the program would never get a "runtime error" indicating a bad usage of the feature: it would either be a compiler/linker error or an arbitrary -- but *deterministic* -- selection of the overload. N2216 allows no "pure virtual" open methods: it requires that you provide a definition for the "top level" declaration. This way you always have an implementation to fall back to. Also its solution has a consequence that a different overrider can be chosen in different loaded DLLs for the same combination of concrete types. This solution is counterintuitive to me and looks similar to the ODR-violation. It is also possible that two DLLs provide a different overrider for the same combination of types. But this is not surprising. The problem of selecting "the right" overrider (whatever it means, given the two-dimensional flexibility) is not solvable. N2216 makes this lack of a good solution very clear. It also shows how other languages fail to solve it (for instance, requiring a "glue code" to be provided by the person who assembles the final program, which defeats the purpose of the "two dimensional flexibility" of open methods. Anyway, the solution for the overrider selection in N2216 is very different from the one in Boost.OpenMethod, so it is incorrect to say that Boost.OpenMethod implements N2216. It does not. It chose a different approach. In this different approach, you can get a runtime error. Worse still, you can sometimes get a runtime error and sometimes an arbitrary overrider, and it is difficult to remember when which happens. N2216 says that a new linker technology would be required to guarantee no-runtime-error. Boost.OpenMethod seems to lack it. As a consequence, we are getting a feature with a lot of type-safety holes. They are plugged by throwing exceptions and calling std::abort(), but they are still holes: you cannot preemptively prevent them, because of the announced flexibility: anyone can extend the type hierarchy or introduce new open-methods at run-time, via linking a DLL. Boost.OpenMethod is inherently type-unsafe and advertises this as flexibility. It is a common practice for library functions to have *preconditions* and to enforce them by calling std::abort() or throwing an exception, but this works only because the users have the means of controlling the correct usage of a function: sqrt(x); And I could have checked before the call that `x` is negative, or I could have made sure that `x` is the result of summing squares. Because I am in control. But in this call: to_string(matrix); I am not in control of satisfying a precondition, because I do not control what other matrix classes have been added to the hierarchy by other users in other DLLs that I may not even be aware of. This is why exceptions or std::abort() are not a good solution anymore. It is no longer a viable precondition. Another difference between Boost.OpenMethod and N2216 is that in the latter a programmer never says that they are overriding a virtual method. The compiler has to deduce the fact from the signatures. Another difference is that N2216 makes it an error to define a parameter as virtual when a corresponding class s not a reference to a polymorphic type. This is a feature (a safety feature). Boost.OpenMethod does not have it: I can pass any type I want in virtual_ptr only to get an abort at run-time: https://godbolt.org/z/b1Y5zMcfh Given this type of a library, where: * A language feature is implemented as a library * The feature is about declaring interfaces (rather than used for internal implementation, such as BOOST_FOREACH) * The feature itself is controversial and not well explored * It cannot be implemented as a library * The feature is complex I would expect a way more of a discussion and explanations in the documentation. More concepts and definitions. The lack of those, makes me further question the soundness of the design. For one instance, the statement, "Multiple and virtual inheritance are supported, with the exception of repeated inheritance." What does it mean "not to support a form of inheritance"? Compiler error? Run-time abort()? Undefined Behavior? I can check what the current implementation does, but is this guaranteed, or is it a happenstance of the today's implementation? Now, let's compare open methods with pattern matching that can distinguish different concrete classes in a type hierarchy. Regardless if they are language features or library emulations. The type-switch (as in wg21.link/n3449) or "match-expression" (as in wg21.link./p2688) is not devoid of similar problems: Someone may add a new class to the hierarchy after I have implemented my match-expression: double get_area(const Shape& shape) { return shape match { Circle: let [r] => 3.14 * r * r; Rectangle: let [w, h] => w * h; _: throw MyLib::Unimplemented(); }; } And if some new class Polygon now inherits from Shape, I will get a throw. But this time, it is plain clear, written down in my code, in what order the candidates will be considered (up-to-bottom, first match). No surprises of the form, "I thought that X would be a better match". Earlier, I made a claim that the authors of N2216 dropped the idea in favor of pursuing pattern matching. I cannot find any citation that would support this. I guess I got that from the fact that two of the authors of N2216 -- Bjarne Stroustrup and Yuriy Solodkyy -- later wrote papers only on pattern matching (wg21.link/n3449, https://www.stroustrup.com/OpenPatternMatching.pdf). Maybe I got this from the ISO C++ mailing lists. If I understood correctly, the idea behind virtual_ptr is that it should be created early to front-load the expensive hash table look-ups. This tells me that even though I have to use a lot of macros in my interfaces, they are still not able to hide the implementation details, and I am exposed to these details through the pieces of advice, such as "pass around virtual_ptr and store them as members". Then I am exposed to more things, like final_virtual_ptr (not to mention unique_virtual_ptr and shared_virtual_ptr). Here's an important point. Boost is known to deliver language features in form of libraries. But the ones I know were to be used inside function bodies: BOOST_FOREACH, BOOST_AUTO. So my callers do not have to know that I am even using them. It is my business. But with Boost.OpenMethod, we are talking about declaring interfaces. When I -- a library author -- make a choice to use a Boost.OpenMethod-style open method, how do I explain all these quirks to my users? Now I am spilling all those implementation details, micro-decisions, limitations to the poor users. Now, I have a more philosophical question. If we declare open-methods (or even pattern-matching-like type switches) non-intrusively on class hierarchies, these hierarchies no longer need to have virtual functions: he polymorphic behavior can now be implemented as open methods. In that case, what is the purpose of declaring class hierarchies? Previously the answer was clear: the types are in the OO-hierarchy because while they have different implementations, they provide the same interface. Now, with open methods they do not provide the same interface. So why put them into a hierarchy? Every library or a language feature or a tool is a burden: you have to learn it, your users have to learn it, you need to debug it. In order for it to be useful, the costs of usage/maintenance must be outweighed by the benefits of using it. I do not see this being the case for Boost.OpenMethod. This is my main objection. I have smaller observations that could be fixed: * There is a number of mysterious but unexplained terms used, such as "guide functions" or "with the virtual_ decorators stripped". * Things are imported into a global namespace (such as virtual_ptr). Don't do it by default. If users want to see virtual_ptr and final_virtual_ptr as keywords, let them type `using namespace boost::openmethod::keywords;`. * The framework behaves differently based on whether my types are copyable or not. Compare: ** https://godbolt.org/z/zKzYzbj3x ** https://godbolt.org/z/Ya8TrE79K For some standard review questions: * I have played with some toy examples using Clang on Compiler Explorer and using GCC on MinGW on Windows. No problems running the examples. * I spent a couple of days in total, studying the docs, re-reading related papers, running examples, filing bugs, and demonstrating the library in my workplace. * I have kept an eye on the papers on open-methods, open type-switch and pattern matching, playing with Mach7 library. Finally, this review may seem very negative, so let me stress again that I appreciate the work Jean-Louis has done to explore the subject. A critique can be seen as a form of appreciation. Also, thanks to Дмитрий for managing the review. Regards, &rzej;
Hi Andrzej, Thank you for the review.
The library claims to solve two problems: the "expression problem" and the "banana-gorilla-jungle problem". In my opinion, the first problem is not a technical problem worth solving at all. [...] Jean-Louis provided a very compelling example of printing a Matrix which is from a OO hierarchy
Instead of "expression problem", it can also be "marketed" as a way to deal with "cross-cutting concerns". The matrix example, or the rendering of an AST as a RPN string, are both cross-cutting concerns.
1. Use OO-style class hierarchies with pattern-matching on top: either wait for the C++ pattern matching, or use a library like Mach7 ( https://github.com/solodon4/Mach7)
I would like to see both open-methods and pattern-matching in the language; and, in the meantime, in Boost. They exist in the same broad space, overlapping to an extent, yet each brings solutions that the other does not. Mach7 and the related papers contain a benchmark that shows that pattern-matching is slower than type-switch, which is slower than open-methods. But I reckon that, in presence of a single value subject to dynamic dispatch, pattern-matching is not horribly slower at all. I guess that, in many real programs, the difference would be unmeasurable.
It is not clear to me what is the relation between Boost.OpenMethod and N2216. Is the goal to implement N2216? Or is it just mentioned for context, or as a loose inspiration?
It started as a strong inspiration. I never implemented the attempt to resolve ambiguities at all cost in YOMM2. I did not implement covariant return types as a tie-breaker, which has been there from the very first paper, because I had reservations about it. The vocabulary (method declarations and method definitions) I use in YOMM2 is inspired more by CLOS and Clojure than by N2216. For OpenMethod, I thought I could get closer to consensus if I brought the library closer to N2216. In a way I did get a consensus: everybody, including myself, seems to strongly dislike "ambiguity is not an error". I already had some disagreements with N2216 (more below) and they grew as the review progressed. I am now strongly leaning towards making the YOMM2 way the default way of handling ambiguities, with N2216 an opt-in.
But note that N2216 calls itself "a report" and the authors never claimed that they had a solution suitable for standardization. It was an exploration.
I see it quite more than that. It is the last of a series of papers describing essentially the same design, with small variations over time. But here we are just trying to guess what is in other people's heads.
But the solution in N2216 has one important property: the program would never get a "runtime error" indicating a bad usage of the feature: it would either be a compiler/linker error or an arbitrary -- but *deterministic* -- selection of the overload.
Ambiguity handling in OpenMethod is deterministic as well. And just like in N2216, which overrider is picked is undocumented.
N2216 allows no "pure virtual" open methods: it requires that you provide a definition for the "top level" declaration. This way you always have an implementation to fall back to.
This is one of the places where I dare disagree with our three luminaries ;-) You have to look hard to find an *example* of a base-method implementation (what you call a "top level declaration"). In N2216: bool intersect(virtual Shape&, virtual Shape&) { } This doesn't look good, it silently returns... what by the way? Isn't this UB? It's a warning at least. Anyway this will make it harder to troubleshoot a missing overrider. In Solodkyy's paper "Simplifying the Analysis of c++ Programs": int eval (virtual const Expr&) { throw std::invalid argument ("eval"); } This is better. I think that it is what most base-method implementations will look like. Again and again. A counter-example is my "animals meet" example: they ignore each other. But it's a made-up example. Reasonable fallback implementations will happen, but they will not be the most frequent. I have the feeling that Stroustrup & co wanted to avoid runtime errors so badly, while also refusing to throw an exception (I'm with them on this), that they convinced themselves that this is the right design. My take is that this is the same situation as calling a pure virtual function. You get a short message, if any, and then abort(). And there is no way to detect such a call through static analysis of a single translation unit. In OpenMethod, you can look at the report member returned by initialize() and find out, early, that you have virtual tuples without overriders. I could implement the N2216 way. What would it get us? Again, users would write silly catch-alls like the above. And then checking the report would be pointless, because I can detect missing overriders, but not silly ones. I am changing my mind about something though. In release variants, by default, I should output a diagnostic. You could opt out via a policy facet if you want minimal footprint. Above is my position on *missing* overriders, which I implemented. Indeed that is not the N2216 way. The N2216 part I implemented when transitioning from YOMM2 to OpenMethod is how *ambiguities* are handled, i.e. bring that part is in line with N2216. I don't see the difference between OpenMethod and N2216. As long as you don't dlopen() a library that adds methods, overriders or classes, one of the ambiguous overriders is called, unspecified but always the same. Call dlopen(), and everything is reshuffled. The only difference is that, OpenMethod being a library, you have to call initialize() again. Since we're at it, I also have doubts about using covariant return types as ambiguity tie-breakers. It's a conditional mechanism. It kicks in only if the return types are classes. It's getting too complicated. When I present YOMM2, I say: "I don't need to explain the resolution rules, you already know them. Same as overload resolution, only it's at runtime". I find it more coherent, less surprising.
It is a common practice for library functions to have *preconditions* and to enforce them by calling std::abort() or throwing an exception, but this works only because the users have the means of controlling the correct usage of a function:
sqrt(x);
And I could have checked before the call that `x` is negative, or I could have made sure that `x` is the result of summing squares. Because I am in control. But in this call:
to_string(matrix);
I am not in control of satisfying a precondition, because I do not control what other matrix classes have been added to the hierarchy by other users in other DLLs that I may not even be aware of. This is why exceptions or std::abort() are not a good solution anymore. It is no longer a viable precondition.
This is a criticism of open-methods in general, correct?
Another difference between Boost.OpenMethod and N2216 is that in the latter a programmer never says that they are overriding a virtual method.
Doesn't it? It's BOOST_OPENMETHOD and BOOST_OPENMETHOD_OVERRIDE. It's more explicit than the N2216 syntax. Which of course I cannot reproduce, because it's a library. Here's another - a better? - one. In N2216, an overrider can even override multiple base-methods. I cannot emulate that. And anyway, what does `next` mean in that case?
Another difference is that N2216 makes it an error to define a parameter as virtual when a corresponding class s not a reference to a polymorphic type. This is a feature (a safety feature). Boost.OpenMethod does not have it: I can pass any type I want in virtual_ptr only to get an abort at run-time: https://godbolt.org/z/b1Y5zMcfh
We discussed this before, a compile-time check used to exist in YOMM2. It was lost when I added support for custom RTTI and non-polymorphic types. Following your remark, I re-introduced the compile-time test in the "review" branch.
I would expect a way more of a discussion and explanations in the documentation. More concepts and definitions. The lack of those, makes me further question the soundness of the design.
The documentation needs improvement, right. I wouldn't drown "normal" users with information on advanced features though. It's not an easy piece to document. The design gives you (or whomever wants it) a flexibility that N2216 doesn't. You can work with non-polymorphic types. Or rather, they will be polymorphic (because open-methods are), without requiring any virtual functions in your hierarchies.
For one instance, the statement, "Multiple and virtual inheritance are supported, with the exception of repeated inheritance." What does it mean "not to support a form of inheritance"? Compiler error? Run-time abort()? Undefined Behavior?
A compile-time error. Consider: #include <boost/openmethod.hpp> #include <boost/openmethod/compiler.hpp> #include <iostream> struct Repeated { virtual ~Repeated() {} }; struct Base1 : Repeated {}; struct Base2 : Repeated {}; struct Derived : Base1, Base2 {}; BOOST_OPENMETHOD_CLASSES(Repeated, Base1, Base2, Derived); BOOST_OPENMETHOD(wrong, (virtual_ptr<Repeated>), void); BOOST_OPENMETHOD_OVERRIDE(wrong, (virtual_ptr<Derived>), void) { std::cout << "Derived\n"; } int main() { boost::openmethod::initialize(); Derived d; Repeated& r = d; wrong(r); } https://godbolt.org/z/6bvTahhqb Thank you for pushing on this though. Having thought about it, I am pretty confident that I can detect this situation in use_classes/BOOST_OPENMETHOD_CLASSES and generate a better error message.
Earlier, I made a claim that the authors of N2216 dropped the idea in favor of pursuing pattern matching. I cannot find any citation that would support this. I guess I got that from the fact that two of the authors of N2216 -- Bjarne Stroustrup and Yuriy Solodkyy -- later wrote papers only on pattern matching (wg21.link/n3449, https://www.stroustrup.com/OpenPatternMatching.pdf). Maybe I got this from the ISO C++ mailing lists.
Stroustrup:
In retrospect, I don’t think that the object-oriented notation (e.g., x.f(y)) should ever have been introduced. The traditional mathematical notation f(x,y) is sufficient. As a side benefit, the mathematical notation would naturally have given us multi-methods, thereby saving us from the visitor pattern workaround. Stroustrup, 2020, Thriving in a Crowded and Changing World: C++ 2006–2020.
I am not a mind-reader, but it seems to me that he still liked open-methods as of 2020, some years after the pattern matching papers.
If I understood correctly, the idea behind virtual_ptr is that it should be created early to front-load the expensive hash table look-ups.
Yes. It's one of the ways of using the library. You can also use an embedded vptr (see with_vptr). There is an ongoing debate about implementing virtual functions the C++ way (embedded vptr) vs the Go/Rust fat pointer. I believe that in practice it cannot be decided either way. Anyway...
This tells me that even though I have to use a lot of macros in my interfaces, they are still not able to hide the implementation details, and I am exposed to these details through the pieces of advice, such as "pass around virtual_ptr and store them as members".
virtual_ptr is not an implementation detail. It's an interface. While turning YOMM2 into OpenMethod, at one point I decided to bring virtual_ptr - an afterthought in YOMM2 - to front stage. One of my motivations was clarity. If you use virtual_, you use it in the base-method declaration, but not in the overriders. That was confusing. Also note that virtual_ptr at all levels resembles N2216 syntax more.
Then I am exposed to more things, like final_virtual_ptr
It is an optimization (similar to make_shared) that gives open-methods the same speed as native virtual functions. It just loads the vptr from a static variable. When an object is created, we don't have to find its type, we know it. Why not give the *option* of using that knowledge?
(not to mention unique_virtual_ptr and shared_virtual_ptr).
Copied from my reply to Joaquin on April 30.
"Design and evaluation of C++ open multi-methods", 5.3. Smart pointers states:
Defining open-methods directly on smart pointers is not possible. In the following example, (1) yields an error, as ptr1 is neither a reference nor a pointer type. The declaration of (2) is an error, because shared_ptr is not a polymorphic object (it does not define any virtual function). Even when shared_ptr were polymorphic, the open-method declaration would be meaningless. A shared_ptr<B> would not be in an inheritance relationship to shared_ptr<A>, thus the compiler would not recognize foo(virtual shared_ptr<B>&) as an overrider.
void foo(virtual shared_ptr<A> ptr1); // (1) error void foo(virtual shared_ptr<A>& ptr2); // (2) error
I don't buy this. How is it different from plain pointers? A Cat* is not in an inheritance relationship to Animal* either: Cat and Animal are, thus there is a standard _conversion_ from one to the other.
My intuition is that virtual smart pointer arguments must be forbidden because it would require the compiler to be aware of the standard library.
I don't want to over-quote myself, please see my full answer in that post.
Now, I have a more philosophical question. [...] Every library or a language feature or a tool is a burden: you have to learn it, your users have to learn it, you need to debug it. In order for it to be useful, the costs of usage/maintenance must be outweighed by the benefits of using it. I do not see this being the case for Boost.OpenMethod.
This is my main objection.
I have the impression that these paragraphs, while interesting, are general considerations about the principle of open-methods, and the value of libraries; and only in the last sentence you connect them to OpenMethod. I don't perceive Boost as a monolithic message on how programming should be done. I see it more as a toolbox, full of *good* *quality* tools, and it is up to users to pick what tool they like to use. Or not.
* Things are imported into a global namespace (such as virtual_ptr). Don't do it by default. If users want to see virtual_ptr and final_virtual_ptr as keywords, let them type `using namespace boost::openmethod::keywords;`.
I won't die on that hill. This has been suggested earlier in the review, and I considered it (positively) before the review even began. Of course I realise it's borderline. Please consider it done.
* The framework behaves differently based on whether my types are copyable or not. Compare:
** https://godbolt.org/z/zKzYzbj3x ** https://godbolt.org/z/Ya8TrE79K
There are a few things going on here. What you should get is a runtime error: unknown class YNode And that is the root of the problem. *Not* constructors. So what's happened? 1. You are compiling with NDEBUG, so you get no checks, and no diagnostics. That is a mistake on my part. I focused too much on the smallest possible footprint in release builds. I will change it to: diagnostic by default, with the possibility to opt out. 2. In the conversion from YOMM2, I introduced a bug. Fixed in the "review" branch, see https://godbolt.org/z/6v8v5ce4b
* I spent a couple of days in total, studying the docs, re-reading related papers, running examples, filing bugs, and demonstrating the library in my workplace.
I am curious, what were the reactions?
Finally, this review may seem very negative, so let me stress again that I appreciate the work Jean-Louis has done to explore the subject. A critique can be seen as a form of appreciation.
I got valuable feedback, which triggered new ideas, in particular about the customization points, and the defaults (like, emit diags even in release mode by default, N2216 and ambiguity handling). And I still have to read Steven Watanabe's review...So I am happy I took the plunge. One final thing: upon re-reading my reply, I realize that it looks like I am willing to change my mind on a lot of things. Should I be sent back to the drawing board then? Am I shooting myself in the foot? So: * I am always open to changing my mind, given good arguments. I also know what my goals are though. * This is the first time my work on open-methods gets a review. I had users and even contributors for a decade. This time I get a review and I find the feedback very valuable. The conversations I am having on the policy/facet system are guiding me to something better, simpler. * Many of the "doubts" I expressed are related to picking the best *defaults* * I intend my library to be production-ready. I also want it to enable research and experimentation. The first motivation for policies was unit tests. Then it was experimenting with memory layouts for dispatch data. Then hashing. And finally, accepting that in the C++ world, there is also a reasonable need to use custom RTTI; to disable exceptions; to operate with limited resources. J-L
J-L, thanks for the reply. I appreciate this discussion. In fact, I believe the biggest value of the Boost Review process is the discussions that get triggered. * I am always open to changing my mind, given good arguments. I also know
what my goals are though.
The above is worth stressing for the wider audience. It is actually a proof of good quality and a sound design when the author refuses to implement some feedback because they have a clear picture of where they want to be, and they detect incompatibilities with the main goal. I appreciate this statement. Indeed, a lot of my criticism goes against open methods in general. I could summarize this as follows. the two dimensional (one dimension is the number of sub-types, the other is the number of methods) run-time flexibility necessarily results with runtime surprises and gotches. Runtime surprises go against a robust system design. Let me clarify: I am perfectly fine with Boost.OpenMethod not implementing N2216. I think you are right that N2216 may have made wrong decisions. What I am raising though, is that from reading the docs, one might get an impression that implementing N2216 was the goal. Therefore, my suggestion is to make it abundantly clear that Boost.OpenMethod does not implement N2216. You also made very good points why N2216 is wrong about some aspects, and why you decided to provide a different solution. I strongly believe these explanations should go into the documentation. Not the tutorials for the novices, of course, but to the "in depth" part. The Boost guidelines for documentation ( https://www.boost.org/development/requirements.html#Documentation) recommend adding a section on the design rationale. One other point, you explain a number of things by referring to YOMM2. This is valuable background information. But when it comes to the documentation or rationale, I think Boost.OpenMethod should be self-contained: I should be able to understand the rationale without having to learn YOMM2. I sort of get the idea behind virtual_ptr, but I still find the end result not acceptable: there is `virtual_ptr` and there is `virual_`. Having the two ways in one library is too much. You asked about the reactions of the audience in my work. They were interested until I showed them that they will get a `std::abort()` whenever a match cannot be found. At that point I think I lost them. In my work we try hard to minimize any run-time surprises. We prefer compile-time errors to run-time errors. Open methods in general (this is not specific to Boost.OpenMethod) prefer run-time errors. And personally, I disagree that open-method run-time errors are like "pure virtual function call". The latter is a very rare case, and you have to try hard to get it. The former, on the other hand, is encouraged by the two-dimensional run-time flexibility. You also asked, what you should do with all this feedback. To a great extent it is up to you. The review manager will tell you, if the library is accepted, which changes are necessary to get the approval, and which observations are just "worth considering". The reviews like mine are only used to guide the manager to make their verdict. Thanks again for your work on open methods. Best regards, &rzej; czw., 8 maj 2025 o 07:33 Jean-Louis Leroy <jl@leroy.nyc> napisał(a):
Hi Andrzej,
Thank you for the review.
The library claims to solve two problems: the "expression problem" and the "banana-gorilla-jungle problem". In my opinion, the first problem is not a technical problem worth solving at all. [...] Jean-Louis provided a very compelling example of printing a Matrix which is from a OO hierarchy
Instead of "expression problem", it can also be "marketed" as a way to deal with "cross-cutting concerns". The matrix example, or the rendering of an AST as a RPN string, are both cross-cutting concerns.
1. Use OO-style class hierarchies with pattern-matching on top: either wait for the C++ pattern matching, or use a library like Mach7 ( https://github.com/solodon4/Mach7)
I would like to see both open-methods and pattern-matching in the language; and, in the meantime, in Boost. They exist in the same broad space, overlapping to an extent, yet each brings solutions that the other does not.
Mach7 and the related papers contain a benchmark that shows that pattern-matching is slower than type-switch, which is slower than open-methods. But I reckon that, in presence of a single value subject to dynamic dispatch, pattern-matching is not horribly slower at all. I guess that, in many real programs, the difference would be unmeasurable.
It is not clear to me what is the relation between Boost.OpenMethod and N2216. Is the goal to implement N2216? Or is it just mentioned for context, or as a loose inspiration?
It started as a strong inspiration. I never implemented the attempt to resolve ambiguities at all cost in YOMM2. I did not implement covariant return types as a tie-breaker, which has been there from the very first paper, because I had reservations about it. The vocabulary (method declarations and method definitions) I use in YOMM2 is inspired more by CLOS and Clojure than by N2216.
For OpenMethod, I thought I could get closer to consensus if I brought the library closer to N2216. In a way I did get a consensus: everybody, including myself, seems to strongly dislike "ambiguity is not an error".
I already had some disagreements with N2216 (more below) and they grew as the review progressed.
I am now strongly leaning towards making the YOMM2 way the default way of handling ambiguities, with N2216 an opt-in.
But note that N2216 calls itself "a report" and the authors never claimed that they had a solution suitable for standardization. It was an exploration.
I see it quite more than that. It is the last of a series of papers describing essentially the same design, with small variations over time. But here we are just trying to guess what is in other people's heads.
But the solution in N2216 has one important property: the program would never get a "runtime error" indicating a bad usage of the feature: it would either be a compiler/linker error or an arbitrary -- but *deterministic* -- selection of the overload.
Ambiguity handling in OpenMethod is deterministic as well. And just like in N2216, which overrider is picked is undocumented.
N2216 allows no "pure virtual" open methods: it requires that you provide a definition for the "top level" declaration. This way you always have an implementation to fall back to.
This is one of the places where I dare disagree with our three luminaries ;-)
You have to look hard to find an *example* of a base-method implementation (what you call a "top level declaration").
In N2216:
bool intersect(virtual Shape&, virtual Shape&) { }
This doesn't look good, it silently returns... what by the way? Isn't this UB? It's a warning at least. Anyway this will make it harder to troubleshoot a missing overrider.
In Solodkyy's paper "Simplifying the Analysis of c++ Programs":
int eval (virtual const Expr&) { throw std::invalid argument ("eval"); }
This is better. I think that it is what most base-method implementations will look like. Again and again. A counter-example is my "animals meet" example: they ignore each other. But it's a made-up example. Reasonable fallback implementations will happen, but they will not be the most frequent.
I have the feeling that Stroustrup & co wanted to avoid runtime errors so badly, while also refusing to throw an exception (I'm with them on this), that they convinced themselves that this is the right design.
My take is that this is the same situation as calling a pure virtual function. You get a short message, if any, and then abort(). And there is no way to detect such a call through static analysis of a single translation unit.
In OpenMethod, you can look at the report member returned by initialize() and find out, early, that you have virtual tuples without overriders.
I could implement the N2216 way. What would it get us? Again, users would write silly catch-alls like the above. And then checking the report would be pointless, because I can detect missing overriders, but not silly ones.
I am changing my mind about something though. In release variants, by default, I should output a diagnostic. You could opt out via a policy facet if you want minimal footprint.
Above is my position on *missing* overriders, which I implemented. Indeed that is not the N2216 way.
The N2216 part I implemented when transitioning from YOMM2 to OpenMethod is how *ambiguities* are handled, i.e. bring that part is in line with N2216. I don't see the difference between OpenMethod and N2216. As long as you don't dlopen() a library that adds methods, overriders or classes, one of the ambiguous overriders is called, unspecified but always the same. Call dlopen(), and everything is reshuffled. The only difference is that, OpenMethod being a library, you have to call initialize() again.
Since we're at it, I also have doubts about using covariant return types as ambiguity tie-breakers. It's a conditional mechanism. It kicks in only if the return types are classes. It's getting too complicated. When I present YOMM2, I say: "I don't need to explain the resolution rules, you already know them. Same as overload resolution, only it's at runtime". I find it more coherent, less surprising.
It is a common practice for library functions to have *preconditions* and to enforce them by calling std::abort() or throwing an exception, but this works only because the users have the means of controlling the correct usage of a function:
sqrt(x);
And I could have checked before the call that `x` is negative, or I could have made sure that `x` is the result of summing squares. Because I am in control. But in this call:
to_string(matrix);
I am not in control of satisfying a precondition, because I do not control what other matrix classes have been added to the hierarchy by other users in other DLLs that I may not even be aware of. This is why exceptions or std::abort() are not a good solution anymore. It is no longer a viable precondition.
This is a criticism of open-methods in general, correct?
Another difference between Boost.OpenMethod and N2216 is that in the latter a programmer never says that they are overriding a virtual method.
Doesn't it? It's BOOST_OPENMETHOD and BOOST_OPENMETHOD_OVERRIDE. It's more explicit than the N2216 syntax. Which of course I cannot reproduce, because it's a library.
Here's another - a better? - one. In N2216, an overrider can even override multiple base-methods. I cannot emulate that. And anyway, what does `next` mean in that case?
Another difference is that N2216 makes it an error to define a parameter as virtual when a corresponding class s not a reference to a polymorphic type. This is a feature (a safety feature). Boost.OpenMethod does not have it: I can pass any type I want in virtual_ptr only to get an abort at run-time: https://godbolt.org/z/b1Y5zMcfh
We discussed this before, a compile-time check used to exist in YOMM2. It was lost when I added support for custom RTTI and non-polymorphic types. Following your remark, I re-introduced the compile-time test in the "review" branch.
I would expect a way more of a discussion and explanations in the documentation. More concepts and definitions. The lack of those, makes me further question the soundness of the design.
The documentation needs improvement, right. I wouldn't drown "normal" users with information on advanced features though. It's not an easy piece to document.
The design gives you (or whomever wants it) a flexibility that N2216 doesn't. You can work with non-polymorphic types. Or rather, they will be polymorphic (because open-methods are), without requiring any virtual functions in your hierarchies.
For one instance, the statement, "Multiple and virtual inheritance are supported, with the exception of repeated inheritance." What does it mean "not to support a form of inheritance"? Compiler error? Run-time abort()? Undefined Behavior?
A compile-time error. Consider:
#include <boost/openmethod.hpp> #include <boost/openmethod/compiler.hpp> #include <iostream>
struct Repeated { virtual ~Repeated() {} };
struct Base1 : Repeated {}; struct Base2 : Repeated {};
struct Derived : Base1, Base2 {};
BOOST_OPENMETHOD_CLASSES(Repeated, Base1, Base2, Derived);
BOOST_OPENMETHOD(wrong, (virtual_ptr<Repeated>), void);
BOOST_OPENMETHOD_OVERRIDE(wrong, (virtual_ptr<Derived>), void) { std::cout << "Derived\n"; }
int main() { boost::openmethod::initialize(); Derived d; Repeated& r = d; wrong(r); }
https://godbolt.org/z/6bvTahhqb
Thank you for pushing on this though. Having thought about it, I am pretty confident that I can detect this situation in use_classes/BOOST_OPENMETHOD_CLASSES and generate a better error message.
Earlier, I made a claim that the authors of N2216 dropped the idea in favor of pursuing pattern matching. I cannot find any citation that would support this. I guess I got that from the fact that two of the authors of N2216 -- Bjarne Stroustrup and Yuriy Solodkyy -- later wrote papers only on pattern matching (wg21.link/n3449, https://www.stroustrup.com/OpenPatternMatching.pdf). Maybe I got this from the ISO C++ mailing lists.
Stroustrup:
In retrospect, I don’t think that the object-oriented notation (e.g., x.f(y)) should ever have been introduced. The traditional mathematical notation f(x,y) is sufficient. As a side benefit, the mathematical notation would naturally have given us multi-methods, thereby saving us from the visitor pattern workaround. Stroustrup, 2020, Thriving in a Crowded and Changing World: C++ 2006–2020.
I am not a mind-reader, but it seems to me that he still liked open-methods as of 2020, some years after the pattern matching papers.
If I understood correctly, the idea behind virtual_ptr is that it should be created early to front-load the expensive hash table look-ups.
Yes. It's one of the ways of using the library. You can also use an embedded vptr (see with_vptr).
There is an ongoing debate about implementing virtual functions the C++ way (embedded vptr) vs the Go/Rust fat pointer. I believe that in practice it cannot be decided either way. Anyway...
This tells me that even though I have to use a lot of macros in my interfaces, they are still not able to hide the implementation details, and I am exposed to these details through the pieces of advice, such as "pass around virtual_ptr and store them as members".
virtual_ptr is not an implementation detail. It's an interface. While turning YOMM2 into OpenMethod, at one point I decided to bring virtual_ptr - an afterthought in YOMM2 - to front stage. One of my motivations was clarity. If you use virtual_, you use it in the base-method declaration, but not in the overriders. That was confusing.
Also note that virtual_ptr at all levels resembles N2216 syntax more.
Then I am exposed to more things, like final_virtual_ptr
It is an optimization (similar to make_shared) that gives open-methods the same speed as native virtual functions. It just loads the vptr from a static variable. When an object is created, we don't have to find its type, we know it. Why not give the *option* of using that knowledge?
(not to mention unique_virtual_ptr and shared_virtual_ptr).
Copied from my reply to Joaquin on April 30.
"Design and evaluation of C++ open multi-methods", 5.3. Smart pointers states:
Defining open-methods directly on smart pointers is not possible. In the following example, (1) yields an error, as ptr1 is neither a reference nor a pointer type. The declaration of (2) is an error, because shared_ptr is not a polymorphic object (it does not define any virtual function). Even when shared_ptr were polymorphic, the open-method declaration would be meaningless. A shared_ptr<B> would not be in an inheritance relationship to shared_ptr<A>, thus the compiler would not recognize foo(virtual shared_ptr<B>&) as an overrider.
void foo(virtual shared_ptr<A> ptr1); // (1) error void foo(virtual shared_ptr<A>& ptr2); // (2) error
I don't buy this. How is it different from plain pointers? A Cat* is not in an inheritance relationship to Animal* either: Cat and Animal are, thus there is a standard _conversion_ from one to the other.
My intuition is that virtual smart pointer arguments must be forbidden because it would require the compiler to be aware of the standard library.
I don't want to over-quote myself, please see my full answer in that post.
Now, I have a more philosophical question. [...] Every library or a language feature or a tool is a burden: you have to learn it, your users have to learn it, you need to debug it. In order for it to be useful, the costs of usage/maintenance must be outweighed by the benefits of using it. I do not see this being the case for Boost.OpenMethod.
This is my main objection.
I have the impression that these paragraphs, while interesting, are general considerations about the principle of open-methods, and the value of libraries; and only in the last sentence you connect them to OpenMethod.
I don't perceive Boost as a monolithic message on how programming should be done. I see it more as a toolbox, full of *good* *quality* tools, and it is up to users to pick what tool they like to use. Or not.
* Things are imported into a global namespace (such as virtual_ptr). Don't do it by default. If users want to see virtual_ptr and final_virtual_ptr as keywords, let them type `using namespace boost::openmethod::keywords;`.
I won't die on that hill. This has been suggested earlier in the review, and I considered it (positively) before the review even began. Of course I realise it's borderline. Please consider it done.
* The framework behaves differently based on whether my types are copyable or not. Compare:
** https://godbolt.org/z/zKzYzbj3x ** https://godbolt.org/z/Ya8TrE79K
There are a few things going on here.
What you should get is a runtime error: unknown class YNode
And that is the root of the problem. *Not* constructors.
So what's happened?
1. You are compiling with NDEBUG, so you get no checks, and no diagnostics. That is a mistake on my part. I focused too much on the smallest possible footprint in release builds. I will change it to: diagnostic by default, with the possibility to opt out.
2. In the conversion from YOMM2, I introduced a bug. Fixed in the "review" branch, see https://godbolt.org/z/6v8v5ce4b
* I spent a couple of days in total, studying the docs, re-reading related papers, running examples, filing bugs, and demonstrating the library in my workplace.
I am curious, what were the reactions?
Finally, this review may seem very negative, so let me stress again that I appreciate the work Jean-Louis has done to explore the subject. A critique can be seen as a form of appreciation.
I got valuable feedback, which triggered new ideas, in particular about the customization points, and the defaults (like, emit diags even in release mode by default, N2216 and ambiguity handling). And I still have to read Steven Watanabe's review...So I am happy I took the plunge.
One final thing: upon re-reading my reply, I realize that it looks like I am willing to change my mind on a lot of things. Should I be sent back to the drawing board then? Am I shooting myself in the foot?
So:
* I am always open to changing my mind, given good arguments. I also know what my goals are though. * This is the first time my work on open-methods gets a review. I had users and even contributors for a decade. This time I get a review and I find the feedback very valuable. The conversations I am having on the policy/facet system are guiding me to something better, simpler. * Many of the "doubts" I expressed are related to picking the best *defaults* * I intend my library to be production-ready. I also want it to enable research and experimentation. The first motivation for policies was unit tests. Then it was experimenting with memory layouts for dispatch data. Then hashing. And finally, accepting that in the C++ world, there is also a reasonable need to use custom RTTI; to disable exceptions; to operate with limited resources.
J-L
participants (3)
-
Andrzej Krzemienski -
Jean-Louis Leroy -
Дмитрий Архипов