Re: [boost] out_ptr's formal review starts now!

On 6/25/19 12:27 AM, Andrey Semashev wrote:

On 6/24/19 11:14 PM, Zach Laine via Boost wrote:

...

On Tue, Jun 18, 2019 at 10:00 AM Zach Laine wrote:

...

On Sun, Jun 16, 2019 at 12:34 AM Zach Laine 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#pre...

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?

In fact, I don't quite understand how shared_ptr can be supported at all since it doesn't have a release() method.

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.

4.1. Actually, what happens if calling reset() in the destructor throws? In particular, what happens with the pointer that is about to be owned by the smart pointer? What if the destructor is called due to another exception?

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.