Thank you Ruben for taking the time to review.
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.
I thought I tried ;-) e.g. https://jll63.github.io/Boost.OpenMethod/#virtual_ptr_extern_vptr. When I reorganize the reference to use groups as suggested by Peter (macros, classes, etc) maybe I should add two categories: Facets and Facet Implementation. It would make the specs of the facets more "findable". Also I am considering changing the terminology to Facet Category / Facet.
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>'"
For that one I can static_assert with a clear message. I will try to strengthen compile-time detection as much as possible. I am also going to add a Troubleshooting Guide.
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.
I would love to switch to C++20 altogether. The internals make heavy use of enable_if. Concepts would be neater. But it feels too early just yet. Also, what is Boost's policy WRT upping standard requirements?
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.
In the "review" branch this is a compile-time error.
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?
I am eyeing resource-tight contexts like embedded programming. Also hierarchies that use with_vptr need not be polymorphic. Or rather, they are polymorphic without using virtual member functions.
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.
I won't die on that hill :-D
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.
Yes I considered that. Probably the direction I'll take.
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.
Hmmm yeah maybe. std::uintptr_t is a bit hard to type...
12. The custom RTTI example uses a virtual destructor.
It's there because std::unique_ptr requires it, not OpenMethod.
This is important because the default policy's is_polymorphic implementation uses std::is_polymorphic, but I don't think it's explained.
But custom_rtti provides its own. That being said, the "review" branch had a bug related to this for one day: virtual_ptr did not channel the test through the policy.
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.
I'm pretty sure it doesn't.
Clang's AST nodes don't have it.
I can imagine scenarios where you allocate objects, not with `new`, but from e.g. deques. In that case you don't need a virtual destructor. Just speculating...