Re: out_ptr's formal review starts now!

From: Louis Dionne (ldionne.cpp_at_[hidden])
Date: 2019-06-26 21:46:51

On Sun, Jun 16, 2019 at 1:35 AM Zach Laine via Boost <boost_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.
>
> out_ptr is a library for making it easy to interoperate between smart
> pointers and traditional C-style initialization and allocation interfaces.
> It also enables doing so in a way that allows library authors to opt into
> speed optimizations for their smart pointers that give them performance
> equivalent to typical C pointers (see benchmarks (
> https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/benchmarks.adoc
> )
> and the Standard C++ proposal for more details).
>
> Online docs can be found here:
>
>  https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr.adoc
>
> The GitHub repository is available here:
>
>  https://github.com/ThePhD/out_ptr
>
> Also, there is an associated standards proposal:
>
> https://thephd.github.io/vendor/future_cxx/papers/d1132.html
>
> We encourage your participation in this review. At a minimum, please state:
>
> - Whether you believe the library should be accepted into Boost
>

I think it should be accepted -- but my accept is weak (see end of post).

> - Your name
>

Louis Dionne

> - Your knowledge of the problem domain
>

I have not had a lot of exposure to C-style APIs and the problem domain the
library is solving. I had not thought about this problem before seeing the
paper.

>
> You are strongly encouraged to also provide additional information:
>
> - What is your evaluation of the library's:
>  * Design
>

The design is simple and minimizes the boilerplate that users have to type,
which I like.

I find the customization point appropriate. I find that a lot of people's
adversity for specialization and opening namespaces is not founded. My
experience so far has been that ADL-based customization points, on the
contrary, have more pitfalls and are harder to use. So I like the proposed
design for customization points.

>  * Implementation
>

I found the implementation to be somewhat difficult to follow, especially
for such a simple utility. There's a lot of indirection layers and it's
unclear to me what's necessary and what's unjustified sophistication. For
example, I would have tried not to have such a deep inheritance hierarchy:

    class inout_ptr_t : public detail::core_inout_ptr_t /* this is either
simple_inout_ptr_t or clever_inout_ptr_t */
        class simple_inout_ptr_t : public base_inout_ptr_impl
            struct base_inout_ptr_impl : base_out_ptr_impl

I understand it allows implementation to be shared, but it's hard to
follow. Perhaps there's no better way to achieve the same level of sharing,
though, so this is not really against the library, just a comment.

There's one thing I really dislike about the implementation, though. It's
the clever aliasing trick. While it is clever, it's unsafe and it adds
complexity to the library. In my opinion, this kind of thing should live
merely in a separate experimental branch, or always be enabled in the cases
where we know for sure it's safe (I think that's never but I'm not sure).

>  * Documentation
>

I found the documentation to be good, however I would have appreciated a
more tutorial-like structure. I found that I was often going back to the
top-level page and browsing down one of the sections, which was a bit
cumbersome. I think a table of contents for the whole documentation
accessible from each page would solve the problem for me.

>  * Tests
>

There appears to be good test coverage in terms of the code being tested,
however the first time I tried using the library on macOS with libc++, I
found a couple of problems. Those were fixed quickly after I reported them
to the author, however it's not clear to me how well tested the library is
outside of Windows. I think that part is still a bit of a work in progress
but the author can correct me.

Also, I think the failure tests need to be pinned down more tightly. Right
now, the tests can pass even when the compilation fails for an unrelated
reason (I found some of those and they were fixed by the author).

>  * Usefulness
>

This is where I'm the least certain. Like I said, I haven't been in a
situation where this utility would have been useful before, and so my view
of the usefulness of the library is tainted by that.

However, I do see the clear benefits the library offers over the manual
equivalent especially in terms of exception safety, and I like that.

> - Did you attempt to use the library? If so:
>  * Which compiler(s)?
>

AppleClang 10 and 11 on MacOS Catalina

>  * What was the experience? Any problems?
>

I had a few problems but I resolved them offline with the author.

> - How much effort did you put into your evaluation of the review?
>

Reading the documentation and the associated standard paper, and reviewing
the implementation at a high level. 4 hours or so total.

Like I said above, I think the library should be accepted. However, my main
reason is that it's a small self-contained utility and Boost seems like a
good place to experiment whether this is actually worth putting into the
Standard. To be clear, I'm less swayed by the problem the library is
solving than by the prospect of using Boost as a testing ground for this
utility before putting it into the Standard.

Thanks to JeanHeyd for the submission,
Louis