out_ptr formal review

From: Gavin Lambert (boost_at_[hidden])
Date: 2019-07-05 03:12:11

My apologies in advance for the light novel.

# Do you think the library should be accepted as a Boost library?

Not in its current form, although I encourage cleaning it up and 
resubmitting it.

# Your knowledge of the problem domain

I've used COM smart pointers (usually CComPtr) a few times, and more 
rarely wrapped some other C or C-like APIs.  (Although mostly pre-C++11, 
requiring something more hand-rolled than unique_ptr.)

# What is your evaluation of the library's:
## Design

The API seems reasonable for both out_ptr and inout_ptr.  (Although also 
see the end of this post.)

Actual uses for inout_ptr outside of a "realloc" type function seems a 
bit rare to me (and such APIs also seem rather rare), but perhaps there 
are other justifying cases I am not aware of, or it's more common in a 
particular domain.

I do like that unspecified function argument evaluation order was 
considered, and I agree that it is better to try to maintain safety 
regardless of evaluation order rather than simply telling users to not 
access the smart pointer in other arguments -- it is a very easy trap 
for users to fall into, especially where the same code that has been 
working forever may suddenly fail with a different compiler version or 
different compiler settings.

(Sequencing of destructor execution with additional && conditionals is 
admittedly a similar problem with no good solution within this design, 
as is explained in the caveats.)

I don't like that it is left unspecified whether release() is called on 
construction or destruction (as that decision is left to the custom 
specialisations), as this potentially reintroduces that problem.  I 
think it should be required that all specialisations behave in the 
following manner:

For out_ptr:
   - construction initialises a raw ptr to nullptr and does not alter 
the smart ptr.
   - destruction resets the smart pointer to the raw ptr (in all cases).
     - this provides a useful guarantee that the smart ptr is always 
reset to nullptr if the called method returns nullptr, even if it had 
some other value previously.

For inout_ptr:
   - construction get()s the current value of the smart ptr in two 
separate raw ptrs (one passed as the function argument, one cached for 
the destructor to use for comparison), but does not release() it.
   - destruction will (only if the raw ptr now has some different value) 
release()s the smart ptr and then reset()s it to the new raw ptr.
     - this makes it safe to use the smart ptr's value in other 
arguments of the method call; unspecified release() timing would have 
made that unsafe.

(There is a slightly dodgy step in there where technically the smart 
pointer could be pointing at something which it thinks it owns, but that 
the called function has already deleted.  However given that any normal 
or exceptional exit path would then call the inout_ptr destructor 
[guaranteed before the smart pointer destructor], which would safely 
release() the pointer without double-deleting it, this seems 
sufficiently safe, and worth it to permit safe argument usage.  The only 
caveat is that usage of the smart pointer sequenced after the method 
call but before the destructor execution [as in additional && 
conditions] is not safe, as it may access deleted memory; however that 
code would never have the correct behaviour anyway and the documentation 
already warns to avoid doing that.)

Perhaps there are some types of smart pointer for which this contract 
wouldn't work (eg. if it is possible for release() to fail, or to 
release a different pointer from what get() returns)?  But then if so 
I'm not convinced it's safe to give them a uniform interface with 
pointers where it is safe, because otherwise inevitably someone is going 
to hit the unspecified-parameter-evaluation-order problem (or assuming 
that it will be nullptr on failure).

Especially given these suggested constraints, I would prefer that a 
simple ADL or traits-type customisation point were added, so that you 
can adapt to smart pointers which merely have different spellings of the 
reset and release methods and don't require deeper customisation -- 
though the existing customisation point could be retained for especially 
weird smart pointer types if needed.  (ADL customisation points are 
superior because, if designed correctly, they would not introduce a 
dependency on Boost.OutPtr, and if named well could theoretically also 
be used by other libraries, as with non-member begin and swap.)

Similarly, while the static_assert check in base_out_ptr_impl 
specifically for std::shared_ptr and boost::shared_ptr seems valuable to 
prevent a significant class of bugs, I question whether it might be 
better handled by checking a type trait or other customisation point 
rather than checking directly for those types, such that it would be 
possible for a user-defined smart pointer type to also participate if it 
has a similar concern.  Or perhaps it should just discourage use of 
shared_ptr at all; there should be little performance difference between 
using shared_ptr directly and using a unique_ptr which is later moved to 
a shared_ptr.  (Since neither can take advantage of the make_shared 
optmisation, and by definition ownership must be unique at that point.)

## Implementation

I haven't verified whether the current implementation exactly follows my 
suggested contract above, as there are a few too many layers of 
indirection and inheritance for me to easily parse.  Simplifying the 
implementation would be nice if possible, especially for out_ptr, to 
improve readability.  (However I think that my description is close at 
least.)

I am strongly opposed to both of the "clever" implementations in the 
library.  They should be completely removed.

I can think of absolutely no justification (performance or otherwise) 
for a "clever" out_ptr.  Due to both the requirement to pass in nullptr 
and to not delete the original smart pointer on construction (as it 
might be used in other parameters), the raw pointer can never have the 
same storage as the smart pointer anyway.  (At least not unless you 
absolutely promise to never use a non-nullptr smart pointer, which 
reduces usefulness, albeit admittedly for a slightly peculiar use case 
[which would have been unsafe when used with raw pointers directly].)

(In theory you could pass in an uninitialised raw pointer and then 
conditionally reset or do nothing depending on whether it still has the 
same uninitialised value after the call, but this both requires 
capturing the actual value at construction and providing a weaker 
contract to the C API, so it seems like a bad idea and a pessimisation 
to me.  Another option is to unconditionally reset() on construction, 
but this sacrifices use in other parameters.)

There is a possible justification for the clever inout_ptr, in that you 
can save two raw pointer copies if you reuse the smart pointer's 
storage, and this does not have the same caveats as with out_ptr.  I am 
not convinced that anyone should care about that (especially in code 
paths also including memory allocation), although it is possible that 
the savings may be more significant with a smart pointer more complex 
than unique_ptr.

Having said that, this "clever" implementation should absolutely never 
be used for any existing smart pointer, especially including 
std::unique_ptr or boost::movelib::unique_ptr.

If there is sufficient justification for its existence at all, then you 
should instead create a new smart pointer (bikeshed: 
boost::out_ptr::fast_ptr) which behaves equivalent to std::unique_ptr 
except that you can steal its raw pointer storage when using inout_ptr. 
This could be made move-compatible with std::unique_ptr for the people 
who want to keep that type in their interfaces -- although that would 
presumably also remove any dubious performance benefit.

As previously brought up in a separate thread, the current review 
implementation also leaks public inheritance of std::tuple, which is 
presumably intended as an empty-base optimisation, but must not be 
public or it will allow inappropriate slicing (and Very Bad Thingsâ„¢).

## Documentation

The documentation seems decent, although being split into many short 
pages as at present can at times make it harder to locate something 
specific.  I would prefer longer single pages divided into sections, but 
that's a matter of taste and not a review condition.

I dislike the assertion that a "well-designed" C API would always set 
the parameter to nullptr on failure.  That is certainly one option (and 
typically a preferred one), but leaving the parameter unchanged is 
another entirely reasonable option, and does not signify "poor design". 
It is only reasonable to claim poor design if the API allocates and sets 
the value, internally deletes it, and then reports failure (while still 
returning the altered and deleted pointer), as this condition cannot be 
handled by out_ptr (or indeed by anything that doesn't understand the 
failure return semantics of the API).

The time axis scale in the benchmarks seems not entirely helpful as a 
lot of the time the difference between two implementations is within one 
axis division of each other.

And again, the magnitude of performance difference sitting in the <1ns 
range in code which inherently implies memory allocation suggests a 
severe case of premature optimisation to me.  (Though perhaps that might 
be more justifiable when used in non-memory handle contexts -- but then 
that might be an abuse of calling it a "pointer".)

## Usefulness

I think that this library or something like it could be quite useful to 
help unify and improve safety when interacting with C-style T** APIs (or 
T* output arguments for some opaque handle type that requires cleanup), 
without requiring exploitation of operator& tricks like CComPtr does.

(Sure, it's trivial to write an RAII wrapper around such APIs, but this 
can become cumbersome when there are hundreds or thousands of such APIs, 
unless you're using a code generator.  And it's easy to accidentally 
forget subtle but important things such as deleting copy operations.)

# Did you try to use the library? With what compiler? Did you have any 
problems?

I did not try it.  However the "clever" implementation is highly 
compiler-and-library-specific and quite aside from its design issues I 
think it would also produce significant portability issues.

# How much effort did you put into your evaluation?

I spent several hours on this review over the course of several days, 
and some related prior discussion in the mailing list.  I read through 
all the documentation and most of the code (though not the tests).

============================
Having said all of the above, I wonder whether the library author has 
considered a different design for this functionality?

Rather than the given example code:

     using bop = boost::out_ptr;

     boost::shared_ptr<mll_driver> axis_driver(nullptr);
     mll_errno err = mll_driver_init(
         bop::out_ptr(axis_driver, mll_driver_free),
         /* additional args ...*/);
     if (err != MLL_SUCESS) {
         // ...
     }

A wrapping lambda syntax could be used instead:

     boost::shared_ptr<mll_driver> axis_driver(nullptr);
     mll_errno err = bop::invoke(
         [&](mll_driver** p) {
             return mll_driver_init(p,
                 /* additional args ...*/); },
         bop::out_ptr(axis_driver, mll_driver_free));
     if (err != MLL_SUCESS) {
         // ...
     }

Here, rather than its current meaning, bop::out_ptr just defines a 
parameter grouping which is then turned into a raw pointer-pointer 
argument for the lambda; it has no destructor-based shenanigans.  (This 
could also be used for other non-pointer argument types -- there just 
needs to be a way for invoke to query the expected API argument type 
from the out_ptr or from the lambda itself.)

This is admittedly more verbose than the current design (although 
bop::invoke could be written as just "invoke" due to ADL, for a slight 
simplification); but it would allow you to more safely control the 
sequencing between the API call and the reassignment of the smart 
pointer -- in particular all of the existing caveats with order of 
evaluations and with if-statement and composite conditional ordering no 
longer apply.  (You can safely "&& axis_driver" above as long as you do 
it outside of the invoke call.)

The API would be called (via the lambda) from a try-catch block inside 
of bop::invoke (or instead of the try-catch an RAII helper can be used 
to perform a try-finally action), and either the existing guarantee of 
not calling reset() or release() until after the call can be maintained 
(thereby allowing the smart pointer to be used by reference within the 
lambda), or it could be forbidden to use the smart pointer inside the 
lambda and the caller could lambda-capture the necessary arguments by 
value instead.  Either way, the caller should be less surprised about 
lifetime issues across a lambda "border", and would quickly discover if 
they did it "wrong" (because the reset/release would either always occur 
before or always occur after argument evaluation, never in mixed order).

invoke would normally be called with only one out_ptr or inout_ptr 
argument (and consequently one argument to the lambda), but could in 
principle be extended to any number of arguments either by nested invoke 
calls or by supporting a variadic form which does the equivalent within 
the library.
It could possibly also be extended to support user-defined argument 
types to do something more complex, by defining a concept interface that 
it uses to interact with the argument, rather than explicit overloads or 
specialisations on those specific types.  This could leave the decision 
of when to reset/release up to the smart-pointer-specific specialisation 
(if that's useful for performance reasons), and without leaving any time 
bombs for the caller.