Result of out_ptr review.
I've compiled the results of the reviews of out_ptr during its recent review. 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. 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. 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
On Mon, Jul 29, 2019 at 8:34 AM Zach Laine via Boost-users
I've compiled the results of the reviews of out_ptr during its recent review.
Thank you very much for the time, dedication, and professionalism that you brought to this review. Regards
Dear Zach, Thank you for the review and feedback. I'll take this all to heart. Sincerely, JeanHeyd Meneide
On Monday, July 29, 2019, Vinnie Falco wrote:
On Mon, Jul 29, 2019 at 8:34 AM Zach Laine wrote:
I've compiled the results of the reviews of out_ptr during its recent review.
Thank you very much for the time, dedication, and professionalism that you brought to this review.
Seconded. Many thanks to Zach for managing the review. I think this can also count as the first successful formal review under the initiative that Zach set up, to give library proposals targeting the standard a guaranteed review manager (for which many of us endorsed). Glen
On 30/07/2019 03:34, Zach Laine wrote:
I've compiled the results of the reviews of out_ptr during its recent review. Many thanks for managing the review process.
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.
I don't think I can agree with that. shared_ptr's reset (or some other kind of smart pointer) is absolutely allowed to throw, and since by design this would be called in out_ptr's destructor, this case needs to be considered.
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)); ... }
That's not legal anyway, since in_out_ptr is incompatible with shared_ptr, as it lacks the ability to release() a uniquely owned raw pointer. Consider instead (with no exceptions already in flight): create_foo(out_ptr(a), out_ptr(b)); The out_ptr destructors are executed in unspecified order. One of them might throw before the other one, and the other one might also throw (but even if it doesn't, is it safe from leaking memory? What if the caller doesn't want std::terminate to be called?).
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.
I find this argument compelling as well, but presumably one of the original goals was to interoperate with existing smart pointer types such as CComPtr or RefPtr. Perhaps that does come at too high a price though. While the lambda-based syntax that I proposed is definitely not as tidy as CComPtr's operator& or OutPtr's current syntax, it does resolve a lot of these safety concerns (all of them, AFAIK). It's unfortunate that C++ lacks a more compact lambda expression syntax.
pon., 29 lip 2019 o 17:35 Zach Laine via Boost
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
participants (6)
-
Andrzej Krzemienski -
Gavin Lambert -
Glen Fernandes -
JeanHeyd Meneide -
Vinnie Falco -
Zach Laine