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.
