|
|
Boost : |
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-06-25 10:20:46
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:
> 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. In any case,
these requirements must be dependable and documented.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk