out_ptr review

From: Glen Fernandes (glen.fernandes_at_[hidden])
Date: 2019-06-30 00:31:13

[Edit: Some changes for clarity within, as it was pointed out on Slack
that some of the phrasing is confusing. Also updated subject.]

Many thanks to JeanHeyd for going through the Boost review process,
and to Zach for managing the review.

Briefly: I cannot justify acceptance for out_ptr just yet in its
current form. I hope to see it return for a new review if the feedback
from this review shapes the design so that some of these concerns are
resolved.

Second: This excludes "inout_ptr" (for which I had even more concerns)
and just covers "out_ptr" which is the simpler and more common use
case of the two.

Below when comparing "what happens under the hood" with out_ptr
against the hand written code that a user would write to use a typical
C out-pointer API with smart pointers, this is what I'm comparing:

[Typical code user would write to use a smart pointer]:

    Type* raw;
    Result result = TheCApi(args..., &raw);
    if (!Success(result)) {
        // failure (e.g. return, or branch and handle error)
    }
    Smart<Type, Deleter> smart(raw, etc...);

[What would happen under the hood when using out_ptr]:

    /* First, 'smart' already has to exist here before TheCApi is called.
       Either as Smart<Type, Deleter> smart;
       or Smart<Type, Deleter> smart(nullptr, etc...); depending on deleter */

    /* User writes: Result result = TheCApi(args..., out_ptr(smart));
        or Result result = TheCApi(args..., out_ptr(smart, etc...));
       depending on deleter */

    Type* raw = nullptr;
    Result result = TheCApi(args..., &raw);
    if (raw != nullptr) {
        smart.reset(raw); // or smart.reset(raw, etc...); depending on deleter
    }
    if (!Success(result)) {
        // failure (e.g. return, or branch and handle error)
    }

Concern 1. Having to define the smart pointer before TheCApi is
called. Ordinarily this might be fine for some smart pointers and
stateful deleters, but it may not be fine or desirable for others
(depending on the deleter, and depending on the use case).

Concern 2. The original user code initializes the smart pointer based
on Success(result). The out_ptr version does so based on whether the
raw out pointer was changed to something non-null by TheCApi. This
might be fine for some cases, but it may not be for others. That it
isn't the same as fundamental logic as the user code bothers me,
though.

Concern 3. The equivalent of the hand written user code is less
overhead. Personally I'm of the opinion that this doesn't matter too,
but here the author does say he has users who cannot abide by it, and
that this facility needs support from e.g. std::unique_ptr to be
viable.  I'm not a fan of needing unique_ptr implementations to add
support for this to make it comparable to some user's hand written
code   (And this also leads to concern 4 below).

Concern 4. The UB optimization that the author wrote because of
Concern 3 is a real concern to him. The UB optimization has no place
in Boost, and one of which doesn't even work with many C++ library
vendors' unique_ptr. (Fortunately the Boost reviews have already
echoed this sentiment and I believe this is going away).

Concern 5. Supporting other custom smart pointers requires effectively
re-implementing out_ptr_t by specialization. I have come to see this
as not viable. (Already mentioned in other reviews, and I echo this.)

Concern 6. C APIs where the deleter (its state, logic, etc.) are
governed by some other result from TheCApi. This is can be worked
around in many cases, but the workaround is clumsy. Or insupportable,
in other cases. i.e. The stateful deleter in the example above is
always initialized with 'etc...' either before-hand via Smart<Type>
smart(nullptr, etc...) or at call site with: TheCApi(args,
out_ptr(etc...)); before analyzing other results of calling TheCApi.

[Concern 7. This concern I had made notes about pre-review has already
been eliminated, when the unconditionally noexcept destructor is now
only conditionally noexcept.]

Other thoughts: The suggestion by Emil lead to an interesting
alternative idea to this problem domain, but I had no time to fully
explore it. I don't think it would address all of these concerns. It
has some other greater appeal, e.g. a way to call C APIs that
materializes the smart pointers as results (along with C API return
value) of an "Invoke-style" thing. Or even a way to transform a C API
with out-pointers into a C++ callable that returns smart pointers
(along with C API return value) via a "Bind-style" thing.

Glen