On Sun, Jun 16, 2019 at 1:35 AM Zach Laine via Boost
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