Dear Andrey, Thank you for your review! Some comments below. On Mon, Jun 24, 2019 at 6:02 PM Andrey Semashev via Boost < boost@lists.boost.org> wrote:
On 6/25/19 12:27 AM, Andrey Semashev wrote:
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.
out_ptr does not need a .release() method. inout_ptr needs a .release() method. The former has no expectation the C function will reallocate, and so will delete the old pointer for you. The latter expects the C function to delete + allocate (reallocate), so it does not delete the old pointer for you. boost.out_ptr does not magically know: it simply makes not passing in the deleter entirely illegal when you use it with boost::/std::shared_ptr. You must pass in the deleter, because 9/10 times you are using a special C function which has a special C destroy/delete function. Forgetting that (and letting shared_ptr assume default_delete<T> on your behalf) is catastrophic. Maybe I can improve the wording here, but the code works as intended and the static assert message is very explicit https://github.com/ThePhD/out_ptr/blob/master/include/boost/out_ptr/detail/b... .
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.
ADL extension points were looked into rather deeply. In fact,
std::out_ptr/inout_ptr's San Diego Committee Meeting discussion had Eric
Fiselier and Titus Winters explicitly requesting that I take a deep dive
into extension points for out_ptr. That deep dive became its own talk at
C++Now 2019, which enumerates why trying to design an ADL extension point
-- niebloid-style or otherwise -- is not a good idea:
https://www.youtube.com/watch?v=aZNhSOIvv1Q&feature=youtu.be&t=3452
In particular, let's go through what I did: let's create an ADL-based
extension point. We make it so that calling boost ::out_ptr::out_ptr(smart,
args...) or boost ::out_ptr::inout_ptr(smart, args...) creates a basic
struct that handles all the details for you. We will ignore that with this
base design we lose performance from not being able to optimize the
structure to reseat the pointer, as that would require more hookable ADL
function calls: this is just to illustrate the primary problem with
overloadable ADL.
You're only required to write a out_ptr_reset( Smart& smart_ptr, T* ptr,
... ) or inout_ptr_reset( Smart& smart_ptr, T* ptr, ... ) function, which
we check for with ADL and then call if it is callable.
The problem with this approach is two-fold: the first is that the first
parameter -- the smart pointer -- is by non-const reference. This is
required so that one can actually modify the smart pointer itself (to call
.reset(), or equivalent functionality). Anyone who derives from your Smart
class publicly will immediately be in the running for ADL call. It's not
the biggest problem and it is fixable by having users not publicly derive
from smart pointer types. However, I will not hold my breath waiting for
the day that stops happening in codebases.
The second problem is much easier to trigger, and there is less of a fix
here with the `...` bit: the rest of the args. You are a developer. You
want to work with a new smart pointer, say:
https://github.com/slurps-mad-rips/retain-ptr
You create one version of out_ptr_reset( sg14::retain_ptr<CType>& smart,
CType* ptr ); -- no problem. It's like you state: it's simple, easy and
effective. But then you have causes where you need to call the other
.reset() overloads with the tag objects, or more. Well, you can
specifically write all 3 or 4 functions to do that. But that means if the
.reset() call changes, you're now responsible for updating your
out_ptr_reset too. Even if it never updates, writing 4+ versions to pass in
the arguments you want to the type? Boo. So you write:
template
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.
The clever one is used. There are config macros for defining it for inout_ptr, but I missed the mark for out_ptr before the review. The safety macros are enumerated and used properly in the feature/boost.review branch of the docs, to address the concerns you brought up about documentation in general: https://github.com/ThePhD/out_ptr/blob/feature/boost.review/docs/out_ptr/con.... (This will not be visible by others using the master branch until post-review, as is standard review practice. All other links in this e-mail are based on master unless explicitly noted otherwise.)
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?
It is marked: https://github.com/ThePhD/out_ptr/blob/master/include/boost/out_ptr/detail/b... It is also marked in the documentation: https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/reference/inout_p... If the .reset() throws, the function transparently tosses that out of the destructor and into the surrounding scope (because it is marked conditionally noexcept). If another exception is already in flight, then you already have bigger issues. Note that by default out_ptr has its output pointer marked as nullptr ("it's value initialization"): this compares true to nullptr and the .reset() call won't be in that case, which means there's no chance to throw to begin with (even if another exception is in flight). With inout_ptr, it is unspecified what may happen because the library can implement an optimization that reseats the pointer directly: at that point, there is no throw. If it doesn't do that optimization, it has a chance to throw based on how the implementation goes about implementing the base functionality. For boost::out_ptr::inout_ptr's implementation specifically, there is only a throw if the smart pointer is non-null coming in and the resulting .reset() call throws. As with all things,using multiple (in)out_ptr's subjects this to order of evaluation issues and makes it a bit easier to hit these edge cases. In practice, I have not received bug reports or seen issues with it, even before I changed the destructor to be not-always-noexcept. I have documented some of the discussion in the new branch, albeit this won't be on the "master" branch that most people see until post-review:
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.
This is a rendering issue with GitHub. It is a security issue to load arbitrary links and dump their contents, so all github "include and display the example here" content is specifically rendered as a link. I have worked around it as best I can. It will go away if the docs are generated by hand (e.g., if it's on the Boost documentation page). I was told that the look/feel of the documentation could be accommodated for, so long as I properly took care of the structure. Looking at other libraries using ASCIIDoc from Boost, I have no reason to believe my library will render any less poorly than the others shown today using the same syntax and markup as my AsciiDoc files do. If you have any other comments, please do let me know; thank you for your time! Sincerely, JeanHeyd Meneide