Boost logo

Boost :

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:
>>>> 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:
>> 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:
>> 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:
>> 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/
>> branch
>> of the docs, to address the concerns you brought up about
>> documentation in
>> general:
>> (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:
>> It is also marked in the documentation:
> 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

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

Boost list run by bdawes at, gregod at, cpdaniel at, john at