Re: out_ptr's formal review starts now!

From: JeanHeyd Meneide (phdofthehouse_at_[hidden])
Date: 2019-06-25 20:02:09

On Tue, Jun 25, 2019 at 6:29 AM Andrey Semashev via Boost <
boost_at_[hidden]> wrote:

> On 6/25/19 1:20 PM, Andrey Semashev wrote:
> > I'm on the fence on this one. I recognize that this is a safeguard for C
> > users. But I don't like that the tool requires the user to do more work
> > than necessary and doesn't allow to specify extra arguments exactly the
> > same way as you would for reset(). It also introduces a dichtomy with
> > other pointers, like unique_ptr, for which, I assume, you don't require
> > an explicit deleter, even with C. I think, I'd prefer no safeguards and
> > more uniform usage of out_ptr, but others may have a different opinion.
> >
>

The users I had appreciated that std::/boost::shared_ptr were caught as
statically asserted errors about this. More than one person at the
in-person review at C++Now asked why it did this, and one of them did not
know that std::/boost::shared_ptr actually reset the deleter when you
didn't specify it (but understood why when they thought through how the
erasure worked). I also had the problem too when I first began to work this
abstraction out to work with more than shared_ptr. The up-front error
vastly helpful, because it is incredibly easy to forget.

If there were a mechanism for making it easy to nullify the requirement in
a safe manner, I would be more than happy to look into that. I did not find
a way that was particularly palatable, even in investigating traits types
(as in char_traits, not as in allocator_traits).

> >  ...
>
> It's not just more explicit, it requires the user to fully reimplement
> > the library if he wants to add support for his incompatible smart
> > pointer. At that poin, why would he even bother with Boost.OutPtr in the
> > first place.
> >
> > Sorry, but I find this extension mechanism unacceptable.
> >

I can understand that it is cumbersome to write most of the specialization.
I opted for this as the extension mechanism because people had interesting
use cases for allowing optimizations that I could not adequately and
generically capture using ADL or traits or similar. For example, at one
point someone described storing a union of either a reference directly to
the pointer inside of something equivalent to a local_shared_ptr<T> or just
the plain old pointer itself, and doing different things based on what the
use count was.

The base implementation works for an extremely large variety of smart
pointers, especially those that follow the existing practice of boost or
standard smart pointers. I don't expect that people will open it up often
unless they have extremely specific optimization needs, at which point I
expect they will need full control.

> I think, an optional optimization is not an adequate solution - it has
> > observable difference in behavior. The library has to give the user a
> > guaranteed behavior that allows him to write code that has some
> > guaranteed effect (e.g. not terminating the program in case of
> > exception). If inout_ptr cannot be implemented on those terms then you
> > have to think what can be reasonably done to make it work. For example,
> > you could add a requirement that reset() doesn't throw. Or that you
> > don't call reset() if the returned pointer didn't change.
>
> Or, you don't do anything at all in the destructor if an exception is in
> flight.
>

     Users already get a noexcept guarantee if `reset()` doesn't throw, as
specified.

     The problematic case is when `reset()` does throw, like with
shared_ptr. I don't think it's useful or fair to impose the comparatively
high overhead of std::uncaught_exceptions()/std::uncaught_exception()
checking in the constructor and destructor to prevent this class of errors.
The most graceful thing that can happen is an exception is thrown from the
.reset(), and being the first one gets caught by the scope just above.
Cleverness by providing the "guarantee" of not calling .reset() if an
exception is in progress means actually leaking memory if the .reset() call
fails: shared_ptr and friends all guarantee in their .reset/constructor
calls will call delete for you if they toss an exception (
http://eel.is/c++draft/util.smartptr.shared#const-10). This means even if
you terminate you have memory safety: removing the .reset() call means
there is no safety anymore, and it has to be built-in to (in)out_ptr for
every type which might have a throwing .reset() call.

    This also means it would require the deleter to be passed in, or we
assume that we can create one with default_delete<T> or whatever the
default deleter happens to be for the class type passed in (which we might
not always know, without demanding additional traits information of
similar).

     I am very certain that specifying anything more than conditional
noexcept is not a good idea.

Sincerely,
JeanHeyd Meneide