Re: Result of out_ptr review.

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-08-07 14:19:09

pon., 29 lip 2019 o 17:35 Zach Laine via Boost <boost_at_[hidden]>
napisał(a):

> I've compiled the results of the reviews of out_ptr during its recent
> review.
>

Thanks Zach for managing the review, and thanks JeanHeydfor submitting the
library.

> Unfortunately, out_ptr is NOT ACCEPTED, though many of the reviewers
> indicated interest in seeing it come back in some future form.
>
> I counted these formal reviews:
>
> Andrey Semashev: reject
> Louis Dionne: accept
> David Sankel: reject
> Phil Endecott: reject
> Glen Fernandes: reject
> Gavin Lambert: reject
>
> Thanks to all of the above reviewers, and to all those who contributed to
> the ongoing discussions surrounding out_ptr, even if they did not have time
> for a full review.
>
> Points against out_ptr raised during the review:
>
> - Many reviewers indicated their puzzlement that this library was proposed
> at all, since they had never seen enough calls to C APIs in their own code
> to motivate the need for out_ptr.
>
> - Some reviewers who did find the motivating cases of the library
> compelling, still said that they would prefer to write wrappers instead of
> using something like out_ptr.
>
> - Multiple reviewers want the library to provide error-reporting help (it
> currently does not).  In particular, for c_func(out_ptr(p)), there was a
> concern about whether p is assigned a value, when c_func() returns a
> failure-status.
>
> - There was concern from several reviewers about the fact that out_ptr's
> types' dtors may throw, though this is only limited to the use of out_ptr
> with shared_ptr and similar smart pointers.  In particular, non-unique_ptr
> uses will never throw in out_ptr's types' dtors, unless the user provides a
> throwing deleter.
>
> - Multiple reviewers disagreed with the use of template specialization as a
> customization mechanism.
>
> - There was widespread concern about the use of potentially UB-invoking
> tricks to get max performance out of the implementation.
>
> - Many reviewers wanted a more typical Boost tutorial introduction in the
> documentation.
>
>
> Now please permit me a digression.
>
> The library attempts to take a call that looks like this (simplest call
> possible):
>
> // CASE 0
> foo * new_foo;
> TODO
> if (!create_foo(&new_foo)) {
>   // etc. ...
> }
>
> And turn it into something as close as possible to this:
>
> // CASE 1
> std::unique_ptr<foo> new_foo;
> if (!create_foo(new_foo)) {
>   // etc. ...
> }
>
> out_ptr does a great job of this lexically:
>
> // CASE 2
> std::unique_ptr<foo> new_foo;
> if (!create_foo(out_ptr(new_foo))) {
>   // etc. ...
> }
>
> If you have not written thousands of lines of code that look like case 0,
> stop for a second and consider what it would be like to drop those
> thousands of raw pointers into your code, and how much you would like to
> replace them all with unique_ptrs.
>
> Now consider what it would take you to write all the wrapping functions
> necessary to make those thousands of calls look like case 1.  You may make
> tooling to generate the wrappers, and that's time consuming.  Writing it by
> hand is even more so.
>
> out_ptr also has the nice property that using it as I have above with
> unique_ptr does not change the meaning of the code, except to ensure that
> new_foo is guaranteed to be treated in a RAII manner.  Thus we get
> increased exception safety, memory leak mitigation, etc. -- y'know, all
> that RAII stuff.  If out_ptr is implemented well, case 2 should produce
> nearly identical object code to case 0, except that case 2 handles calling
> new_foo's dtor at the right time.
>
> Note that because of this, you have the same relationship to the C API in
> all three cases.  That is, you need to check for the success value returned
> from create_foo(), and you need to know what create_foo()'s contract is in
> the face of errors -- did it write to new_foo or not?  out_ptr is how you
> get from raw pointers to RAII ones, not how you solve all programming
> problems related to calling functions.
>
> Hopefully that motivates the use case for out_ptr, even if you don't need
> it in your own code.  Hopefully that also allays concerns that out_ptr is
> not doing enough -- doing more is outside the library's scope.
>

This is a very useful summary. It really belongs in the documentation of
out_ptr.

>
> Now, on to exceptions in out_ptr's types' dtors.  I heard a lot of
> discussion about this during the review.  To me, concerns about what
> happens when an out_ptr or an inout_ptr throws in its dtor misses the point
> entirely, because it is concerned with fixing corner cases we should not
> even care to support.  Instead of worrying about the semantics of
> exceptions in those kinds of situations, we should instead be asking why we
> want to support user code like this:
>
> ... } catch {
>   create_foo(in_out_ptr(my_shared_ptr_to_a_foo));
>   ...
> }
>
> In other words, we don't really care about the dtor exceptions, unless:
> - there is an active exception already, and
> - we are also calling a C API to create a new object (a possible, but
> certainly atypical thing to do when handling an exception), and
> - we're catching this new object in a shared_ptr-like smart pointer that
> might throw on construction.
>
> Who writes code like that?  Few people, perhaps none.  It's not impossible
> to see code like that, but until there are well-motivated cases for this,
> maybe leave it for a later version of the library.
>

This question comes up once in a while, and it is not tied to out_ptr.
There is a number of libraries that want to use destructors for other
things than releasing resources. I do not necessarily agree with the above
reasoning, but I think it would be beneficial if we were able to come up
with a general guidance on this in Boost, and then just refer to it when
reviewing a library like out_ptr.

Regards,
&rzej;

> In other words, the question should have been "Why support anything other
> than nothrow-constructible RAII types like unque_ptr?"  Note that I find
> this argument compelling for the WG21-facing version of this work too.
>
> To summarize:  I think the first four bullet points listed at the beginning
> of this post are, or could easily be made to be, irrelevant.
>
> So, that leaves some issues still outstanding:
> - figure out how to provide customization points (I'm actually fine with
> the current approach...),
> - eliminate the UB-dangerous code, or make it opt-in,
> - rework the tutorial portion of the docs, and
> - many assorted doc cleanups.
>
> That does not seem like an unreasonable amount of stuff to resolve in
> post-review acceptance readiness.
>
> I bring all this up because I do in fact hope that JeanHeyd brings this
> back for a future review, and I hope that my arguments here will convince
> some of you that this library is a useful thing to have around.
>
> I waited until now to say any of these things in an attempt to remain
> impartial during the review; I've been bothered by seeing previous review
> managers clearly represent a pro-acceptance position from the start.
>
> Thanks to JeanHeyd for his submission.  A no-vote from this group is a
> tough thing, and I hope that the feedback he has received takes some of the
> sting out.
>
> Zach
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>