Re: [OpenMethod] review starts on 28th of April

From: Jean-Louis Leroy (jl_at_[hidden])
Date: 2025-05-07 04:17:00

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...