Re: out_ptr's formal review starts now!

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-06-25 10:29:08

On 6/25/19 1:20 PM, Andrey Semashev wrote:
> 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_at_[hidden]> 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#prevents-errors-with-shared_ptr-resetting-the-deleter 
>>>
>>>>
>>>>
>>>> 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 <typename T, typename... Extras>
>> 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/7e1ef14e30d3b2654dcc577eb312fa7665edbc2e/include/boost/out_ptr/out_ptr.hpp#L25 
> 
> 
>> 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/config.adoc. 
>>
>> (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/base_out_ptr_impl.hpp#L95 
>>
>> It is also marked in the documentation:
>> https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/reference/inout_ptr.adoc#destructors 
>>
> 
> 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.

Or, you don't do anything at all in the destructor if an exception is in 
flight.

> In any case, 
> these requirements must be dependable and documented.