Re: [boost] out_ptr's formal review starts now!

On 6/25/19 11:02 PM, JeanHeyd Meneide wrote:

On Tue, Jun 25, 2019 at 6:29 AM Andrey Semashev via Boost mailto:boost@lists.boost.org> 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.