Re: out_ptr's formal review starts now!

From: JeanHeyd Meneide (phdofthehouse_at_[hidden])
Date: 2019-06-25 00:25:11

Dear Andrey,

     Thank you for your review! Some comments below.

On Mon, Jun 24, 2019 at 6:02 PM Andrey Semashev via Boost <
boost_at_[hidden]> wrote:

> On 6/25/19 12:27 AM, Andrey Semashev wrote:
> > 1. I did not inderstand the example with shared_ptr:
> >
> >
> https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/examples.adoc#prevents-errors-with-shared_ptr-resetting-the-deleter
> >
> >
> > How does out_ptr know that a custom deleter is needed in this particular
> > example? Or does out_ptr always require a deleter when used with
> > shared_ptr?
>
> In fact, I don't quite understand how shared_ptr can be supported at all
> since it doesn't have a release() method.
>

out_ptr does not need a .release() method. inout_ptr needs a .release()
method. The former has no expectation the C function will reallocate, and
so will delete the old pointer for you. The latter expects the C function
to delete + allocate (reallocate), so it does not delete the old pointer
for you.

boost.out_ptr does not magically know: it simply makes not passing in the
deleter entirely illegal when you use it with boost::/std::shared_ptr. You
must pass in the deleter, because 9/10 times you are using a special C
function which has a special C destroy/delete function. Forgetting that
(and letting shared_ptr assume default_delete<T> on your behalf) is
catastrophic. Maybe I can improve the wording here, but the code works as
intended and the static assert message is very explicit
<https://github.com/ThePhD/out_ptr/blob/master/include/boost/out_ptr/detail/base_out_ptr_impl.hpp#L57>
.

> > 2. I think customization by specializing out_ptr_t and inout_ptr_t is
> > not adequate, as it basically means reimplementing most of the library.
> > I would prefer if there was a limited set of operations that could be
> > customized (either as a single customization point, like a traits class,
> > or a set of ADL-found functions), which would be used by both out_ptr_t
> > and inout_ptr_t. I'd like to emphasize that that abstraction layer
> > should be minimal and only deal with interfacing with a particular
> > smart-pointer. It should not implement storing the extra arguments for a
> > reset() call and unpacking them for the call. Or it should not implement
> > conversion operator T*() and associated machinery. All that should
> > remain implemented in Boost.OutPtr.
> >
> > Additionally, I would like to point out that adding support for out_ptr
> > should require zero or next to zero added cost in terms of dependencies,
> > compile time, implementation effort and so forth. For example, using a
> > few simple ADL-found functions as a customization point requires no
> > dependency on Boost.OutPtr (no extra includes) on the client side.
>

ADL extension points were looked into rather deeply. In fact,
std::out_ptr/inout_ptr's San Diego Committee Meeting discussion had Eric
Fiselier and Titus Winters explicitly requesting that I take a deep dive
into extension points for out_ptr. That deep dive became its own talk at
C++Now 2019, which enumerates why trying to design an ADL extension point
-- niebloid-style or otherwise -- is not a good idea:
https://www.youtube.com/watch?v=aZNhSOIvv1Q&feature=youtu.be&t=3452

In particular, let's go through what I did: let's create an ADL-based
extension point. We make it so that calling boost ::out_ptr::out_ptr(smart,
args...) or boost ::out_ptr::inout_ptr(smart, args...) creates a basic
struct that handles all the details for you.  We will ignore that with this
base design we lose performance from not being able to optimize the
structure to reseat the pointer, as that would require more hookable ADL
function calls: this is just to illustrate the primary problem with
overloadable ADL.

You're only required to write a out_ptr_reset(  Smart& smart_ptr,  T* ptr,
... ) or inout_ptr_reset(  Smart& smart_ptr, T* ptr, ... ) function, which
we check for with ADL and then call if it is callable.

The problem with this approach is two-fold: the first is that the first
parameter -- the smart pointer -- is by non-const reference. This is
required so that one can actually modify the smart pointer itself (to call
.reset(), or equivalent functionality). Anyone who derives from your Smart
class publicly will immediately be in the running for ADL call. It's not
the biggest problem and it is fixable by having users not publicly derive
from smart pointer types. However, I will not hold my breath waiting for
the day that stops happening in codebases.

The second problem is much easier to trigger, and there is less of a fix
here with the `...` bit: the rest of the args. You are a developer. You
want to work with a new smart pointer, say:
https://github.com/slurps-mad-rips/retain-ptr

You create one version of out_ptr_reset( sg14::retain_ptr<CType>& smart,
CType* ptr ); -- no problem. It's like you state: it's simple, easy and
effective. But then you have causes where you need to call the other
.reset() overloads with the tag objects, or more. Well, you can
specifically write all 3 or 4 functions to do that. But that means if the
.reset() call changes, you're now responsible for updating your
out_ptr_reset too. Even if it never updates, writing 4+ versions to pass in
the arguments you want to the type? Boo. So you write:

template <typename... Extras>
out_ptr_reset(sg14::retain_ptr<CType>& smart, CType* ptr, Extras&&...
extras);

But that's CType specific. You've got at least 4 C types you're playing
with: why stick to just that?

template <typename T, typename... Extras>
out_ptr_reset(sg14::retain_ptr<T>& smart, T* ptr, Extras&&...  extras);

The above function has now just become a black hole for all other function
calls remotely like it. If you placed this in a header, you are in for a
world of hurt: anyone who uses sg14::retain_ptr<T> -- or publicly derives
from it -- has a chance to pick up your overload, because your overload
accepts literally anything. This means you either must go back to writing
each overload explicitly by hand, or coming up with constraints, or finding
ways of hiding this function from _other people's_ ADL, not just your own.

This is not a design that sat well with me, and it did not work when I
tried it and other iterations out in my more popular libraries to test the
waters. The current design is much safer, even if it is more explicit and
requires more knowledge. Maybe one day the C++ standard will accept Matt
Calabrese's p1292 <http://wg21.link/p1292> which will make it much easier
to avoid these problems of having a single, flat overload resolution space
to duke it out with other functions.

But that's not today's C++.

> 3. The implementation seems to contain two versions (simple and clever).
> > I'm not sure if the clever one is used and what's the advantage of it.
> > If it's not used, it should be removed/moved to a work-in-progress
> > branch. Please, no non-functional cruft for review or in branches for
> > public consumption.
>

    The clever one is used. There are config macros for defining it for
inout_ptr, but I missed the mark for out_ptr before the review. The safety
macros are enumerated and used properly in the feature/boost.review branch
of the docs, to address the concerns you brought up about documentation in
general:
https://github.com/ThePhD/out_ptr/blob/feature/boost.review/docs/out_ptr/config.adoc.
(This will not be visible by others using the master branch until
post-review, as is standard review practice. All other links in this e-mail
are based on master unless explicitly noted otherwise.)

> 4. Since out_ptr_t/inout_ptr_t destructor may throw, it must be marked
> > as such.
>
> 4.1. Actually, what happens if calling reset() in the destructor throws?
> In particular, what happens with the pointer that is about to be owned
> by the smart pointer? What if the destructor is called due to another
> exception?
>

It is marked:
https://github.com/ThePhD/out_ptr/blob/master/include/boost/out_ptr/detail/base_out_ptr_impl.hpp#L95
It is also marked in the documentation:
https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/reference/inout_ptr.adoc#destructors

     If the .reset() throws, the function transparently tosses that out of
the destructor and into the surrounding scope (because it is marked
conditionally noexcept). If another exception is already in flight, then
you already have bigger issues. Note that by default out_ptr has its output
pointer marked as nullptr ("it's value initialization"): this compares true
to nullptr and the .reset() call won't be in that case, which means there's
no chance to throw to begin with (even if another exception is in flight).
With inout_ptr, it is unspecified what may happen because the library can
implement an optimization that reseats the pointer directly: at that point,
there is no throw. If it doesn't do that optimization, it has a chance to
throw based on how the implementation goes about implementing the base
functionality. For boost::out_ptr::inout_ptr's implementation specifically,
there is only a throw if the smart pointer is non-null coming in and the
resulting .reset() call throws.

     As with all things,using multiple (in)out_ptr's subjects this to order
of evaluation issues and makes it a bit easier to hit these edge cases. In
practice, I have not received bug reports or seen issues with it, even
before I changed the destructor to be not-always-noexcept. I have
documented some of the discussion in the new branch, albeit this won't be
on the "master" branch that most people see until post-review:

> 5. The docs have a few typos ("C+11", "+T**", "A user may (partially)
> > template specialize the customize") and are possibly limited by GitHub's
> > asciidoc renderer. For instance, all examples are shown as links. I
> > would recommend reworking and re-generating the docs so that they
> > resemble other Boost docs more closely.
>

     This is a rendering issue with GitHub. It is a security issue to load
arbitrary links and dump their contents, so all github "include and display
the example here" content is specifically rendered as a link. I have worked
around it as best I can. It will go away if the docs are generated by hand
(e.g., if it's on the Boost documentation page). I was told that the
look/feel of the documentation could be accommodated for, so long as I
properly took care of the structure. Looking at other libraries using
ASCIIDoc from Boost, I have no reason to believe my library will render any
less poorly than the others shown today using the same syntax and markup as
my AsciiDoc files do.

     If you have any other comments, please do let me know; thank you for
your time!

Sincerely,
JeanHeyd Meneide