Boost logo

Boost :

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:

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.

Boost list run by bdawes at, gregod at, cpdaniel at, john at