Re: out_ptr's formal review starts now!

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-06-24 21:27:16

On 6/24/19 11:14 PM, Zach Laine via Boost wrote:
> On Tue, Jun 18, 2019 at 10:00 AM Zach Laine <whatwasthataddress_at_[hidden]>
> wrote:
> 
>> On Sun, Jun 16, 2019 at 12:34 AM Zach Laine <whatwasthataddress_at_[hidden]>
>> wrote:
>>
>>> Dear Boost community,
>>>
>>> The formal review of JeanHeyd Meneide's out_ptr library starts Sunday,
>>> June 16th
>>> and ends on Wednesday, June 26th.
>>
>> Just a friendly reminder that this review is happening now.  Please
>> consider posting a review of this library.  It's quite small and quite
>> useful.
> 
> Another reminder that this review is ongoing.  The review period ends in
> two days, and we so far have no submissions.  Please post a review in you
> have a chance.  Again, it is quite small, and thus easy to review!

Disclaimer: This is not an in-depth review. I just read the docs and 
glanced over implementation and had some comments.

1. I did not inderstand the example with shared_ptr:

https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/examples.adoc#prevents-errors-with-shared_ptr-resetting-the-deleter

How does out_ptr know that a custom deleter is needed in this particular 
example? Or does out_ptr always require a deleter when used with shared_ptr?

2. I think customization by specializing out_ptr_t and inout_ptr_t is 
not adequate, as it basically means reimplementing most of the library. 
I would prefer if there was a limited set of operations that could be 
customized (either as a single customization point, like a traits class, 
or a set of ADL-found functions), which would be used by both out_ptr_t 
and inout_ptr_t. I'd like to emphasize that that abstraction layer 
should be minimal and only deal with interfacing with a particular 
smart-pointer. It should not implement storing the extra arguments for a 
reset() call and unpacking them for the call. Or it should not implement 
conversion operator T*() and associated machinery. All that should 
remain implemented in Boost.OutPtr.

Additionally, I would like to point out that adding support for out_ptr 
should require zero or next to zero added cost in terms of dependencies, 
compile time, implementation effort and so forth. For example, using a 
few simple ADL-found functions as a customization point requires no 
dependency on Boost.OutPtr (no extra includes) on the client side.

3. The implementation seems to contain two versions (simple and clever). 
I'm not sure if the clever one is used and what's the advantage of it. 
If it's not used, it should be removed/moved to a work-in-progress 
branch. Please, no non-functional cruft for review or in branches for 
public consumption.

4. Since out_ptr_t/inout_ptr_t destructor may throw, it must be marked 
as such.

5. The docs have a few typos ("C+11", "+T**", "A user may (partially) 
template specialize the customize") and are possibly limited by GitHub's 
asciidoc renderer. For instance, all examples are shown as links. I 
would recommend reworking and re-generating the docs so that they 
resemble other Boost docs more closely.

Bottom line: The idea looks interesting. I don't often have the need for 
such a tool, but when I do, I'd like to have it. However, the 
implementation, especially the design wrt. integration with other 
(incompatible) smart pointers looks not well thought out. The docs need 
some work. If I had to vote, I'd say reject in the current state with 
the hope that the author improves the design for the second submission.

Thank you to the author for the submission.