Hi Everyone,
The library review starts still in two weeks, but I wanted to give some
early feedback.
First, I wanted to thank Jean-Louis for exploring the subject, writing and
sharing the library, and for submitting it for the Boost Review. My
feedback is based on the reading of the documentation, I haven't played
with the library just yet.
[1]. Animals as examples
There are already a number of things in the library that suggest it might
not be suitable for production use (I describe them below), but using class
Animal as explanatory examples amplifies the impression that this is going
to be a toy library. Half of the books I have read on OO use the Animal (or
Employee) example, but never in my C++ career did I have to use OO to model
something close to "animal". I know they are just examples, but picking
one so detached from programming reality is in my estimation harmful. The
arithmetic expression example is way better. I, for one, use OO for
representing dynamically-configurable parts of the applications, such as
plugins or user-selectable sorting or selection predicates.
[2]. The motivation
In C++ we already have a dynamic-dispatch tool for "closed set of methods
with open set of types": virtual functions. We also have a dynamic-dispatch
tool for "open set of methods with closed set of types": the visitation of
std::variant. You need to provide a motivating example (more convincing
than the zoo) that needs exactly this: "open set of methods with the open
set of types". The arithmetic expression example doesn't do the job: there
is a fixed and well known list of concrete types.
Also, the fact that Bjarne Stroustrup wanted to add open methods long ago
is insufficient a motivation.
[3]. Are open methods compatible with static type checking?
It looks that when one of the two things -- number of types, number of
methods -- is known statically, the compiler can verify if the programmer
didn't forget to take some type or method into account. (Well, sometimes we
get the "pure virtual function call" error, but these are very rare
cases.)
The same seems not to be the case for open methods. What this library
advertises as flexibility would be the source of many programmer bugs
detected only at runtime, possibly by end users. Imagine that programmer
Alice is adding her new type A and plugs it into open methods known to her,
and at the same time programmer Bob in another file adds a new open method
B known only to him. Bob doesn't know about class A, and Alice doesn't know
about open method B.
The ISO C++ proposals quoted in the docs mentioned solving this by a
mandated or encouraged linker error. Is it possible for this library to
diagnose the incomplete specification of an open method in the
`initialize()` function?
The docs use an expression, "when an error is encountered [...]". But note
that in C++ we often use the term "error" to express the situation where a
_correct_ program encounters a hostile situation in the environment (lack
of resources, bad user input). But openmethod docs actually mean something
different: a programmer bug. It is very easy for a programmer to plant a
bug in this flexible design.
[4] In a similar vein, the decision to select an arbitrary specialization
of an open method upon a tie, is another runtime surprise: not even a
reported error. For multi methods it is not even clear how a static check
for exhaustive unambiguous specialization should work.
[5] It is concerning that so many things are defined in the global
namespace. The docs say, "First we need to include the library’s main
header. It defines a few macros, and injects a name - virtual_ptr - in the
global namespace." But then the reference part says that virtual_ptr is in
the namespace boost::openmethod. Who is right? The fact that as common a
name as `next` is reserved inside open methods is also uncomfortable. Why
not use a namespace-scoped name?
Next, the docs use the using-directive: `using namespace
boost::openmethod;`. This is a bad idea: both for real programs and for the
documentation: it is not clear if the library was designed with namespaces
in mind. Also, all the examples define open methods in the global
namespace. I instead would like the docs to show how to declare open
methods in my namespace, as this is a far more probable use case.
[6] Friendship
It puzzles me how the notion of friendship can be combined with this idea
of flexibility in open methods. It is my understanding that open methods
are added on top of the existing classes. That is: classes do not have to
know that some open method will be added in the future. In other words,
open methods are non-intrusive, right? So how would a class know who to
grant the friendship to?
Also, the examples use the declaration `friend struct`, even though we are
talking about methods. I think docs should mention the existence of these
structs more prominently.
[7] Function signatures
Some macros in the library use function types in declarations:
`BOOST_OPENMETHOD_OVERRIDERS(poke)<void(std::ostream&, virtual_ptr<Cat>)>`
Others specify the type separately:
`BOOST_OPENMETHOD_OVERRIDE(value, (virtual_ptr<Plus> node), int)`
Is there a reason for this? Can the clever use of macros not offer the same
function signature syntax?
[8] Passing virtual pointers to const objects
If `virtual_ptr<Cat>` is an analogue of this pointer, could we see an
example in the docs that shows a virtual pointer to const objects? It would
be `virtual_ptr<const Cat>`, I guess?
[9] Is this library intended for production use?
The library comes with a lot of clever macros, a lot of pointer type
templates, a lot of new syntax and runtime surprises (mentioned earlier). I
am concerned about how the debugging would look if something goes wrong
with the usage of this library.
Would you recommend it for use in production systems? Or is it for
experimentation?
[10] Minor things
Section Custom RTTI has sentence:
> Now we can include the "core" header
and then header `boost/openmethod/core.hpp` is not included. Is it a bug?
Regards,
&rzej;
