On 6/25/19 3:25 AM, JeanHeyd Meneide via Boost wrote:
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.
I see.
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.
That's true for C functions, but not necessarily for C++. Yes, C++ can return a smart pointer already, but there may be reasons not to (e.g. to keep ABI minimal and not tied to a specific C++ version). I'm on the fence on this one. I recognize that this is a safeguard for C users. But I don't like that the tool requires the user to do more work than necessary and doesn't allow to specify extra arguments exactly the same way as you would for reset(). It also introduces a dichtomy with other pointers, like unique_ptr, for which, I assume, you don't require an explicit deleter, even with C. I think, I'd prefer no safeguards and more uniform usage of out_ptr, but others may have a different opinion.
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.
Just one out_ptr_reset.
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.
If that someone implements a specialized smart pointer, I don't see the problem. And that's a good thing, since that someone gets Boost.OutPtr support for free. If that someone uses inheritance not adequately, that's a problem with that someone's design, which is what needs to be fixed.
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.
No, if I had to, I would create one version like this: namespace sg14 { template< typename T, typename OperationT > void out_ptr_reset(retain_ptr< T >& p, T* ptr, OperationT op) { p.reset(ptr, op); } } because retain_ptr::reset() always takes an operation tag. Ideally, I would restrict OperationT with SFINAE or concepts to retain_object_t/adopt_object_t tags, if they were designed more friendly (e.g. derived from a common base). But I wouldn't have to, because the generic version: namespace boost::out_ptr { template< typename T, typename... Args > void out_ptr_reset(T& p, typename T::element_type* ptr, Args&&... args) { p.reset(ptr, std::forward< Args >(args)...); } } would work just fine. (Just a thought: it may be even better to replace typename T::element_type* with decltype(p.operator->()).) These overloads are the integration glue between Boost.OutPtr and the specific smart pointers. They are supposed to know the details about the smart pointer and thus can have the required set of arguments and restrict them appropriately.
template
out_ptr_reset(sg14::retain_ptr<T>& smart, T* ptr, Extras&&... extras); The above function has now just become a black hole for all other function calls remotely like it. If you placed this in a header, you are in for a world of hurt: anyone who uses sg14::retain_ptr<T> -- or publicly derives from it -- has a chance to pick up your overload, because your overload accepts literally anything.
Not anything, but retain_ptr. If anyone calls out_ptr_reset with retain_ptr (or a derived pointer), he will get exactly what he wanted, and this is a good thing. Also, note that ADL-found functions, although in my opinion are the best way, are not the only option.
This is not a design that sat well with me, and it did not work when I tried it and other iterations out in my more popular libraries to test the waters. The current design is much safer, even if it is more explicit and requires more knowledge.
It's not just more explicit, it requires the user to fully reimplement the library if he wants to add support for his incompatible smart pointer. At that poin, why would he even bother with Boost.OutPtr in the first place. Sorry, but I find this extension mechanism unacceptable.
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.
Seems like not for out_ptr: https://github.com/ThePhD/out_ptr/blob/7e1ef14e30d3b2654dcc577eb312fa7665edb...
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.)
Well, I used master for review, as it was the link posted in the initial review announcement.
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...
I see.
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.
Well, no, not really. It is not unreasonable to expect that the function, whose argument you're wrapping with out/inout_ptr, throws. out/inout_ptr need to have a correct behavior on destruction while an exception is in flight. By correct I mean the one that doesn't end with std::terminate().
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.
I think, an optional optimization is not an adequate solution - it has observable difference in behavior. The library has to give the user a guaranteed behavior that allows him to write code that has some guaranteed effect (e.g. not terminating the program in case of exception). If inout_ptr cannot be implemented on those terms then you have to think what can be reasonably done to make it work. For example, you could add a requirement that reset() doesn't throw. Or that you don't call reset() if the returned pointer didn't change. In any case, these requirements must be dependable and documented.