Re: out_ptr review

From: JeanHeyd Meneide (phdofthehouse_at_[hidden])
Date: 2019-06-30 20:52:31

Dear Glen Fernandes,

     Thank you for your review.

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.

A fair concern, solving this one requires an entirely different library
approach. I think it would be doable if we had better reflection facilities
in C++, or just creating a set of macros or functions which would wrap a
call easier (template <auto fp> in C++20 would make such wrappers less
verbose and perhaps not require as much code).

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

This was indeed concerning, but with a little reporting from Loius Dionne I
was able to get it fixed during the review. It works with VC++, Dinkumware,
libc++ (the current version, not the master branch), and libstdc++ on OSX,
Linux and Windows machines with the Big 3 compilers (plus a few flavors of
MinGW).

This isn't to say this should change people's review of the library or that
they need to review the feature/boost.review branch; at this point it's
more of a mental tracker for me to know what is and isn't fixed.

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

This is a fair assessment. But, at the behest of everyone during the
review, I spent a lot of time mashing on the internals to create a
secondary extension point (that sits on top of the current one). There were
two choices: a facade type, or a traits (similar to allocator_traits) type.
ADL was already proven incredibly insufficient and easy to mess up from
several field tests of that API, and a policy type (e.g. similar to
char_traits) was also disastrously hard to implement.

The traits type is simpler after a large amount of work on the internals. A
demonstration of the struct specialization vs. traits type specialization
is visible here:

Struct Specialization:
https://github.com/ThePhD/out_ptr/blob/feature/boost.review/examples/source/customization.traits.handle.cpp
Traits Specialization:
https://github.com/ThePhD/out_ptr/blob/feature/boost.review/examples/source/customization.handle.cpp

This doesn't alleviate earlier concerns about coupling: I could only
achieve proper de-coupling with an ADL-like solution or one that forces
certain members to be defined in some way on the class that is being used.
Neither were palpable in interface nor prevented errors.

Even if the traits type is shorter, I would not be comfortable proposing it
for the std::, but maybe for Boost if it goes that direction. The struct
specialization has field experience: the traits type was finished literally
3 hours ago. There are also minor storage size benefits to the struct
specialization route that one can take advantage of, decreasing register
usage by 1 or 2 or stack space consumption by 1 or 2 pointers. The good
news is that both the traits type and the struct specialization can exist
together without problem, and adding the traits type later provided no API
and no ABI breakage (other than decreasing the size for an
already-specialzed pointer, at which point that's on the author to make an
informed decision about whether they would wish to migrate).

Benchmarks are still being run with the traits types to see if it has any
significant consequences for today's compilers.

> 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.
>
>
I don't know what to do about this concern. out_ptr is not infallible in
all aspects: just a very useful tool. I spent a lot of time chasing after
wrappers or perfectly encapsulating APIs for C and it never materialized. I
have hope with the Reflection TS, maybe metaclasses and some degree of
markup or shared agreement, this might become easier. Or, at the very
least, not so developer-time costly. Unfortunately, the world of C APIs is
a very vast and very not-standard landscape; it will be a long while before
automatic wrapping to enable multiple-return and automated
std::expected/Boost.Outcome (presumably, there are other return type
choices) to be a thing for even the most visible C APIs.

Sincerely,
JeanHeyd Meneide