Re: out_ptr's formal review starts now!

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-06-25 22:05:56

On 6/25/19 11:02 PM, JeanHeyd Meneide wrote:
> On Tue, Jun 25, 2019 at 6:29 AM Andrey Semashev via Boost 
> <boost_at_[hidden] <mailto:boost_at_[hidden]>> wrote:
> 
>      > 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.
> 
> 
>  Â Â Â Â  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 overhead would be close to a library function call, since the 
functions return a piece of the current thread state. It's not zero, but 
it's not expensive either.

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

You mean, catch and suppress any exceptions thrown by reset() and assume 
it had freed the pointer before throwing? First, this is a requirement 
on reset(), which has to be documented. shared_ptr notwithstanding.

Second, I don't think suppressing the exception when there is no other 
exception in flight is an expected behavior. If the code involving a 
function call with out_ptr argument completed without exceptions, any 
user would assume that all is fine, and the returned pointer is properly 
saved in the resulting smart pointer. I think, you will still have to 
check whether there is an outer exception in flight and let the 
exception from reset() propagate only if there isn't one.

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

Not calling reset() on outer exception makes sense if you require that 
the function that threw it either didn't allocate or freed the memory 
before the exception propagated out of the function. In other words, if 
the function fails, it must clean up the mess after itself before 
returning, exception or not. This is not an unreasonable requirement, 
and in fact, this is the only behavior I consider reasonable in the 
context of raw pointers.

Out of all of the possible solutions brought up so far, this one seems 
the most logical one to me. I seem to remember some real APIs that 
modify the out pointers on failure, but even then they free any 
allocated memory before returning. Meaning that analyzing or using the 
out pointers on failure is not always safe or useful.

> This 
> means even if you terminate you have memory safety:

I don't care about memory leaks if std::terminate() is called. :)

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

This simply means you're not going to support throwing reset(), because 
that means std::terminate(). At this point you might as well document 
this as a requirement and enforce in the implementation. No need for a 
conditional noexcept.

That would make shared_ptr unsupported. Which might be an acceptable 
loss, given that C-style APIs cannot return a shared_ptr (meaning a 
pointer, which is owned by another shared_ptr). From the caller's 
standpoint, the returned pointer is unique, so the caller should be 
using unique_ptr to receive the pointer. He may later use that 
unique_ptr to initialize a shared_ptr, but that would be out of 
Boost.OutPtr scope.