On Tue, Jun 18, 2019 at 10:00 AM Zach Laine <whatwasthataddress@gmail.com> wrote:

On Sun, Jun 16, 2019 at 12:34 AM Zach Laine <whatwasthataddress@gmail.com> wrote:

...

Dear Boost community,

The formal review of JeanHeyd Meneide's out_ptr library starts Sunday, June 16th and ends on Wednesday, June 26th.

Just a friendly reminder that this review is happening now. Please consider posting a review of this library. It's quite small and quite useful.

Another reminder that this review is ongoing. The review period ends in two days, and we so far have no submissions. Please post a review in you have a chance. Again, it is quite small, and thus easy to review! Zach

On 6/25/19 12:27 AM, Andrey Semashev wrote:

On 6/24/19 11:14 PM, Zach Laine via Boost wrote:

...

On Tue, Jun 18, 2019 at 10:00 AM Zach Laine <whatwasthataddress@gmail.com> wrote:

...

On Sun, Jun 16, 2019 at 12:34 AM Zach Laine <whatwasthataddress@gmail.com> wrote:

...

Dear Boost community,

The formal review of JeanHeyd Meneide's out_ptr library starts Sunday, June 16th and ends on Wednesday, June 26th.

Just a friendly reminder that this review is happening now.  Please consider posting a review of this library.  It's quite small and quite useful.

Another reminder that this review is ongoing.  The review period ends in two days, and we so far have no submissions.  Please post a review in you have a chance.  Again, it is quite small, and thus easy to review!

Disclaimer: This is not an in-depth review. I just read the docs and glanced over implementation and had some comments.

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.

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.

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.

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?

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.

Bottom line: The idea looks interesting. I don't often have the need for such a tool, but when I do, I'd like to have it. However, the implementation, especially the design wrt. integration with other (incompatible) smart pointers looks not well thought out. The docs need some work. If I had to vote, I'd say reject in the current state with the hope that the author improves the design for the second submission.

Thank you to the author for the submission.

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 <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/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.

On Tue, Jun 25, 2019 at 6:29 AM Andrey Semashev via Boost < boost@lists.boost.org> wrote:

On 6/25/19 1:20 PM, Andrey Semashev wrote:

...

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.

The users I had appreciated that std::/boost::shared_ptr were caught as statically asserted errors about this. More than one person at the in-person review at C++Now asked why it did this, and one of them did not know that std::/boost::shared_ptr actually reset the deleter when you didn't specify it (but understood why when they thought through how the erasure worked). I also had the problem too when I first began to work this abstraction out to work with more than shared_ptr. The up-front error vastly helpful, because it is incredibly easy to forget. If there were a mechanism for making it easy to nullify the requirement in a safe manner, I would be more than happy to look into that. I did not find a way that was particularly palatable, even in investigating traits types (as in char_traits, not as in allocator_traits).

...

...

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.

I can understand that it is cumbersome to write most of the specialization. I opted for this as the extension mechanism because people had interesting use cases for allowing optimizations that I could not adequately and generically capture using ADL or traits or similar. For example, at one point someone described storing a union of either a reference directly to the pointer inside of something equivalent to a local_shared_ptr<T> or just the plain old pointer itself, and doing different things based on what the use count was. The base implementation works for an extremely large variety of smart pointers, especially those that follow the existing practice of boost or standard smart pointers. I don't expect that people will open it up often unless they have extremely specific optimization needs, at which point I expect they will need full control.

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.

Users already get a noexcept guarantee if `reset()` doesn't throw, as specified. The problematic case is when `reset()` does throw, like with shared_ptr. I don't think it's useful or fair to impose the comparatively high overhead of std::uncaught_exceptions()/std::uncaught_exception() checking in the constructor and destructor to prevent this class of errors. The most graceful thing that can happen is an exception is thrown from the .reset(), and being the first one gets caught by the scope just above. Cleverness by providing the "guarantee" of not calling .reset() if an exception is in progress means actually leaking memory if the .reset() call fails: shared_ptr and friends all guarantee in their .reset/constructor calls will call delete for you if they toss an exception ( http://eel.is/c++draft/util.smartptr.shared#const-10). This means even if you terminate you have memory safety: removing the .reset() call means there is no safety anymore, and it has to be built-in to (in)out_ptr for every type which might have a throwing .reset() call. This also means it would require the deleter to be passed in, or we assume that we can create one with default_delete<T> or whatever the default deleter happens to be for the class type passed in (which we might not always know, without demanding additional traits information of similar). I am very certain that specifying anything more than conditional noexcept is not a good idea. Sincerely, JeanHeyd Meneide

On 26/06/2019 08:02, JeanHeyd Meneide wrote:

I can understand that it is cumbersome to write most of the specialization. I opted for this as the extension mechanism because people had interesting use cases for allowing optimizations that I could not adequately and generically capture using ADL or traits or similar. For example, at one point someone described storing a union of either a reference directly to the pointer inside of something equivalent to a local_shared_ptr<T> or just the plain old pointer itself, and doing different things based on what the use count was.

The specialisation syntax in C++ is fundamentally flawed, and in my view, should absolutely never be used by anybody. One such example of this is that (especially for header-only libraries, although with care this can also be used for compiled libraries) it is very useful to be able to allow multiple versions of a library to co-exist through external namespacing: namespace v1 { #include <library_v1/lib.hpp> } namespace v2 { #include <library_v2/lib.hpp> } Most of the time this Just Works™; one of the cases where this completely falls over is that in this scenario it is impossible for the library to define any specialisations in the std namespace (or indeed any other namespace outside of its internal bubble, including the global namespace). Using ADL is much more convenient; explicit policy arguments can be used as a fallback when ADL is not possible.

The problematic case is when `reset()` does throw, like with shared_ptr. I don't think it's useful or fair to impose the comparatively high overhead of std::uncaught_exceptions()/std::uncaught_exception() checking in the constructor and destructor to prevent this class of errors. The most graceful thing that can happen is an exception is thrown from the .reset(), and being the first one gets caught by the scope just above. Cleverness by providing the "guarantee" of not calling .reset() if an exception is in progress means actually leaking memory if the .reset() call fails: shared_ptr and friends all guarantee in their .reset/constructor calls will call delete for you if they toss an exception ( http://eel.is/c++draft/util.smartptr.shared#const-10). This means even if you terminate you have memory safety: removing the .reset() call means there is no safety anymore, and it has to be built-in to (in)out_ptr for every type which might have a throwing .reset() call.

Throwing another exception during unwinding can (at least in some implementations) clobber the original exception, which will disguise the actual source of the problem for logging or debugging purposes. Having said that, neither choice is a great one; ideally if you know you're already unwinding then you wouldn't call a throwing reset() but you still either have to call a (possibly also throwing) deleter or you will get a memory leak if the original exception does get caught without termination somewhere higher up the call stack. Since I assume it's harder to extract the appropriate deleter than to just call reset(), it's probably better to continue doing that. On 26/06/2019 10:05, Andrey Semashev wrote:

Not calling reset() on outer exception makes sense if you require that the function that threw it either didn't allocate or freed the memory before the exception propagated out of the function. In other words, if the function fails, it must clean up the mess after itself before returning, exception or not. This is not an unreasonable requirement, and in fact, this is the only behavior I consider reasonable in the context of raw pointers. That's fine -- if it was the function itself which throws the exception. (And that shouldn't normally be the case because it's a C-style API.)

But what if the function has two output parameters? Memory is allocated for both and returned as a raw pointer by the function. One of the out_ptrs is then destroyed and calls reset(), and this throws. The other out_ptr is then destroyed as well, and now it has a problem; it either has to call reset() as well and possibly throw yet again, or leak memory. If it calls reset() and it does throw, then terminate() will be called and the memory leak isn't a problem after all. If it calls reset() and it doesn't throw, then there's no memory leak (assuming that the original reset() call cleaned up the memory before throwing, as it is supposed to) -- and if the original exception is caught upstream and handled, then the process can continue on its merry way without issues (assuming it can fix whatever caused the original exception). If it doesn't call reset(), then it guarantees a memory (and possibly also a more serious resource) leak -- which won't matter if the process terminates, but does if the original exception gets caught and "handled". Similar problems can occur if it was an out_ptr and some other complex-destructor object used in the argument list of the original function rather than two out_ptrs.

That would make shared_ptr unsupported. Which might be an acceptable loss, given that C-style APIs cannot return a shared_ptr (meaning a pointer, which is owned by another shared_ptr). From the caller's standpoint, the returned pointer is unique, so the caller should be using unique_ptr to receive the pointer. He may later use that unique_ptr to initialize a shared_ptr, but that would be out of Boost.OutPtr scope. I'm somewhat inclined to agree with this. It *must* be illegal to use a non-unique shared_ptr with inout_ptr, for example; and it barely makes sense to use a unique one. So *in principle* it would be better to only allow unique_ptr. Having said that, the way that shared_ptr handles deleters is more convenient to that of unique_ptr, which might be an important consideration.

(I'm sad that the standard doesn't provide a type-erased deleter form of unique_ptr, in addition to the current form. I know it's not hard to make one, but it would be better if you didn't have to.)

On 26/06/2019 19:18, Dominique Devienne wrote:

On Wed, Jun 26, 2019 at 2:25 AM Gavin Lambert wrote:

...

The specialisation syntax in C++ is fundamentally flawed, and in my view, should absolutely never be used by anybody.

One such example of this is that (especially for header-only libraries, although with care this can also be used for compiled libraries) it is very useful to be able to allow multiple versions of a library to co-exist through external namespacing:

namespace v1 { #include <library_v1/lib.hpp> } namespace v2 { #include <library_v2/lib.hpp> }

Most of the time this Just Works™; one of the cases where this completely falls over is that in this scenario it is impossible for the library to define any specialisations in the std namespace (or indeed any other namespace outside of its internal bubble, including the global namespace).

Using ADL is much more convenient; explicit policy arguments can be used as a fallback when ADL is not possible.

This subject is one I've struggled with, going back and forth in my own closed-source libraries between traits, functions, combination, what not; and that I never felt happy about. Do you know of any good treatment on the subject that details the pros and cons of the various approaches? I've seen it mentioned in Boost reviews as well, notably the Boost.Convert one, with Steven especially, but I clearly lack the depth to fully grasp it. Any good sources on that subject?

Not that I'm aware of, but I'm not really an expert on the subject either; it's just an occasional pet peeve of mine. It's something that would easily be solvable if the language allowed scoped declarations; for example, instead of declaring: } // whoops, we have to close our namespace namespace std { // and hope that this is ::std, but it might not be template<> struct hash<my_ns::Type> { ... }; } namespace my_ns { // and back we go again If you could instead just declare: template<> struct ::std::hash<Type> // my_ns::Type, since still inside my_ns { ... }; This would be immune from the above problem, and much much more convenient to use. But sadly we apparently don't live in a sane world. (It's more debatable whether my_ns should still be in scope inside the body of the specialization. Convenience says yes; consistency says no. Compromise suggests perhaps only my_ns::Type is visible, due to being a template argument.) There have been a few standards proposals to fix this for std::hash in particular by doing something different. But currently it looks like the standard in general is heading down the track of defining even more things that need to be specialised, rather than the reverse. I'm not aware of any general fix being proposed, but I also don't really follow these things.

On Thu, Jun 27, 2019 at 12:48 AM JeanHeyd Meneide via Boost < boost@lists.boost.org> wrote:

I have concerns about all of the ways we do extension in C++ currently (that was the point of the talk I linked in the originating thread).

Thanks. I missed/overlooked that. For reference, that post was [1] and the talk's video is at [2]. --DD [1] https://lists.boost.org/Archives/boost/2019/06/246411.php [2] https://www.youtube.com/watch?v=aZNhSOIvv1Q&feature=youtu.be&t=3452

On 26/06/2019 20:26, Andrey Semashev wrote:

...

Most of the time this Just Works™;

I'd say, most of the time it doesn't work. Including any foreign headers, including standard library headers, in lib.hpp will break the program at some stage, either compilation or linking.

True, although there are workarounds for that; the simplest being that if you already #included everything that the library header does outside of the namespace (except for those things that you do want inside the namespace), then include guards will usually prevent the standard headers being included inside the namespace. (assert.h being a notably annoying exception, as that traditionally lacks guards for some inexplicable reason.) Of course, this also means that you need to defeat the guards of the library itself -- but this typically happens for free if it uses #pragma once guards rather than explicit #defines. And macro definitions in the library can be a problem as well. So you're right, it's not universally applicable, though it can be very useful in specific circumstances. I don't know whether modules will make this scenario better or worse. :)

I'm not saying I like specialization as a way of customization though.

There's exactly two places where a specialization of C<T> can be written: immediately below C, or immediately below T. Anywhere else can give you extreme grief. Of course, ADL isn't much better in that regard, except that you don't have to mess with namespaces as much. (Although actually calling ADL functions correctly can be troublesome in its own way.)

...

But what if the function has two output parameters?  Memory is allocated for both and returned as a raw pointer by the function.  One of the out_ptrs is then destroyed and calls reset(), and this throws. The other out_ptr is then destroyed as well, and now it has a problem; it either has to call reset() as well and possibly throw yet again, or leak memory.

Good point. In that case I see no good solution, given that on error there may be garbage in the out pointers.

To clarify, I meant that the function itself succeeds (and so does not return garbage), but then cleanup of the function arguments is where the exception is thrown. Especially as this occurs in unspecified order. A similar problem can occur if construction of one of the function arguments fails -- if the out_ptr hasn't been constructed yet, then it won't be destructed either and there's no change in behaviour. If it has, then it will be destructed and will try to reseat its raw pointer into a smart pointer -- which will only work correctly for out_ptr if it always initialises its raw pointer to nullptr explicitly, as mentioned below. For inout_ptr it's not a problem -- it just release()d a pointer and then reset() the same one right back again. If the exception occurs after the function call full-expression, then the raw pointers have been successfully saved into smart pointers, and there shouldn't be a problem any more. Similarly if it occurs before the call expression starts. If the function itself throws, it is its duty to avoid leaking memory (basic guarantee) and to not output deleted pointers (either by explicitly setting them to nullptr or leaving them at the same value as on input). Conversely, it's out_ptr's responsibility to always provide a nullptr as input. For inout_ptr, that responsibility falls to the caller to provide a valid possibly-not-nullptr pointer.

On 27/06/2019 09:46, Louis Dionne wrote:

I find the customization point appropriate. I find that a lot of people's adversity for specialization and opening namespaces is not founded. My experience so far has been that ADL-based customization points, on the contrary, have more pitfalls and are harder to use. So I like the proposed design for customization points.

Perhaps I'm missing something, but as I read it the proposed design for customization is "specialize out_ptr itself". Which could alternatively be written as "reimplement the entire library yourself". So I'm not sure I agree that this could be called a "customization point"...

I found the implementation to be somewhat difficult to follow, especially for such a simple utility. There's a lot of indirection layers and it's unclear to me what's necessary and what's unjustified sophistication. For example, I would have tried not to have such a deep inheritance hierarchy:

class inout_ptr_t : public detail::core_inout_ptr_t /* this is either simple_inout_ptr_t or clever_inout_ptr_t */ class simple_inout_ptr_t : public base_inout_ptr_impl struct base_inout_ptr_impl : base_out_ptr_impl

Related: why does out_ptr inherit from std::tuple? Surely this should be a has-a relation, not an is-a relation? Granted this does allow for a zero-base-size optimization, which is useful as most of the time it would be an empty tuple, but it still feels wrong (and would allow inappropriate slicing if you try to pass an out_ptr as a tuple parameter, since it's public inheritance).

There's one thing I really dislike about the implementation, though. It's the clever aliasing trick. While it is clever, it's unsafe and it adds complexity to the library. In my opinion, this kind of thing should live merely in a separate experimental branch, or always be enabled in the cases where we know for sure it's safe (I think that's never but I'm not sure).

I tend to agree. While strict-aliasing rules are a bit of a pain in the butt, sneaking around behind them and poking private members of unique_ptr and friends doesn't seem like a good idea, even if it does improve benchmarks. And "UB-based optimization" does not inspire confidence.

On Thu, Jun 27, 2019 at 12:23 AM Gavin Lambert via Boost < boost@lists.boost.org> wrote:

Granted this does allow for a zero-base-size optimization, which is useful as most of the time it would be an empty tuple, but it still feels wrong (and would allow inappropriate slicing if you try to pass an out_ptr as a tuple parameter, since it's public inheritance).

That's a mistake from mixing class and struct: it should be all "class" and the tuple base should be marked protected. I've fixed it on the feature/boost.review branch.

...

There's one thing I really dislike about the implementation, though. It's the clever aliasing trick. While it is clever, it's unsafe and it adds complexity to the library. In my opinion, this kind of thing should live merely in a separate experimental branch, or always be enabled in the cases where we know for sure it's safe (I think that's never but I'm not sure).

I tend to agree. While strict-aliasing rules are a bit of a pain in the butt, sneaking around behind them and poking private members of unique_ptr and friends doesn't seem like a good idea, even if it does improve benchmarks.

And "UB-based optimization" does not inspire confidence.

I will pull request a friend class of a forward declaration for boost::movelib::unique_ptr to make it not have to go through UB. There is nothing I can do for standard pointers other than to give up the optimization or to use the UB. (This is one of the reasons it was proposed to the standard, asides from being an existing practice with wide implementation experience.) Sincerely, JeanHeyd

Am 27.06.19 um 09:27 schrieb Gavin Lambert via Boost:

On 27/06/2019 18:11, JeanHeyd Meneide wrote:

...

On Thu, Jun 27, 2019 at 12:23 AM Gavin Lambert wrote:

...

And "UB-based optimization" does not inspire confidence.

There is nothing I can do for standard pointers other than to give up the optimization or to use the UB. (This is one of the reasons it was proposed to the standard, asides from being an existing practice with wide implementation experience.)

Correct me if I'm wrong, but the clever implementation appears to depend on several #defines which are entirely undocumented.

Furthermore, one of them selects between whether the private members of std::unique_ptr have the pointer as the "first" or "second" member (with assumption that the first member is then also pointer-sized), with no other options.  This does not seem like the sort of thing that users should be required to define or otherwise know at all -- especially in code that might be compiled against multiple compilers or multiple standard library implementations.  (And while there is a sanity check, it is disabled by default, and also merely asserts "oops, I corrupted your memory" after the fact.)

And the "clever" version of inout_ptr is enabled by default, with all of these undocumented choices.

I sincerely pity the codebase that includes this code.

Avoidance of a few picoseconds copying a raw pointer twice is simply not worth this sort of thing.

I don't even get why there is a performance benefit at all. base_out_ptr: - Copy address of smart ptr - Set internal ptr to NULL - API sets internal_ptr - out_ptr calls reset:   - check smart ptr   - delete smart ptr   - copy new ptr --> 3 ptr copies, 1 NULL copy, 2 check, 1 delete clever_out_ptr: - Copy address of smart ptr - Copy old smart ptr - Copy address of smart ptr (aliased) - API sets  smart ptr - check old ptr - delete old ptr --> 4 Ptr copies, 1 check, 1 delete Looks pretty much the same, or did I miss anything? What I also dislike is the convoluted design and confusing description. See the docs here: https://github.com/ThePhD/out_ptr/blob/master/docs/out_ptr/caveats.adoc#poor... seem to be misleading: The first paragraph makes me wonder what to do with my smart pointer. Do I have to set it to NULL? The implementation seems to be good: It uses a temporary pointer, which is set to NULL, passes that to the API and calls reset on the smart pointer with whatever the value is now. This works for APIs changing the pointer and ones not touching it (on failure). So why confuse the user? The 2nd paragraph is similar: Why not give the same guarantees as with the manual implementations: Do the same as above but call release instead of initializing with NULL. For such "misbehaving" APIs the user must then call `release` on the smart pointer in case of failure.

On Thu, Jun 27, 2019 at 3:27 AM Gavin Lambert via Boost < boost@lists.boost.org> wrote:

On 27/06/2019 18:11, JeanHeyd Meneide wrote:

...

On Thu, Jun 27, 2019 at 12:23 AM Gavin Lambert wrote:

...

And "UB-based optimization" does not inspire confidence.

There is nothing I can do for standard pointers other than to give up the optimization or to use the UB. (This is one of the reasons it was proposed to the standard, asides from being an existing practice with wide implementation experience.)

Correct me if I'm wrong, but the clever implementation appears to depend on several #defines which are entirely undocumented. ... And the "clever" version of inout_ptr is enabled by default, with all of these undocumented choices.

Yes, that was an oversight on my part. It's been corrected in the post-review branch, with full documentation: https://github.com/ThePhD/out_ptr/blob/feature/boost.review/docs/out_ptr/con... The library prefers the fast version by default. It has a configuration option to choose where the clever optimization might look for the pointer value. Given feedback, I think it would be better to maybe have it off by default: that's a small change to flipping how the configuration macros work right now. So far the only reported failures come from libc++ using this clever optimization. This was patched in to be fixed in the post-review branch, but I can't merge such changes to Master until the review is over. Sincerely, JeanHeyd

Dear Phil Endecott, Thank you for your review. There has been a consistent view that there should be beautiful, hand-written wrappers around all of the C functions that you or your application care about. I am convinced these wrappers are both fundamentally flawed and wrong for users. Let's look at your Frambuffers example: class Framebuffer: boost::noncopyable { GLuint handle; public: Framebuffer() { glGenFramebuffers(1, &handle); } ~Framebuffer() { glDeleteFramebuffers(1, &handle); } }; To start, boost::out_ptr::out_ptr accomodates non-pointer handles, the same as std::unique_ptr accommodates them. Your code can be rewritten using a std::unique_ptr<GLuint, frame_buffer_deleter>: struct gl_handle { GLuint handle; friend bool operator ==(GLuint, nullptr); friend bool operator ==(nullptr, GLuint); friend bool operator ==(GLuint, GLuint); friend bool operator !=(GLuint, nullptr); friend bool operator !=(nullptr, GLuint); friend bool operator !=(GLuint, GLuint); }; struct frame_buffer_deleter { using pointer = gl_handle; void operator( gl_handle handle){ glDeleteFramebuffers(handle.handle, 1); } }; class Framebuffer { std::unique_ptr<GLuint, frame_buffer_delete>; public: Framebuffer() { glGenFramebuffers(1, boost::out_ptr::out_ptr<GLuint>(handle)); } }; "That's so much more code, why would anyone do that?!" you ask. For the following reasons: - There's no more destructor to write. If this class ever got marginally more complicated than just be a specifically-named unique_ptr, allocation of the framebuffer may succeed but an exception can be thrown, locking driver resources because you never deleted them: http://coliru.stacked-crooked.com/a/910aa40650884efb and http://coliru.stacked-crooked.com/a/48126c474d033c5d both demonstrate that. There is a reason why the rule of zero -- by R. Martinho Fernandes and included in Scott Meyers's book and spoken about around the world -- became so popular and important in C++11. - Reusable: right now I have programmed to specifically write to your framebuffer example. The code I wrote to do this in reality (and ported to multiple platforms, including for Vulkan and DirectX) used unique_ptr to hold the handle and I could make it handle creating more than 1 framebuffer by adding a sized deleter. Note that such a deleter makes it a strong type: unique_ptr<GLuint[], sized_deleter> != unique_ptr<GLuint, regular_deleter>. - Your code hides it in the internals of Framebuffer and hard-codes it: if you were to ever change the class, you would need to change the ctor, dtor, and anywhere else that the assumption takes place, or write an entirely new class "Framebuffer*s*". If written in the same style of your other one, it would be vulnerable to the same problems. - Bonus Points: boost::noncopyable is no longer needed, just the unique pointer which already renders the class move-only. There are similar problems with your PNG example. All of this goes to show that while you can write cute one-off examples that seem simpler, they do not scale to the fundamental reality that is handing resources, dealing with exceptions, and more in C++ APIs. out_ptr scales far beyond what you just wrote, handles the fact that someone can change their pointer type from `handle` to `handle[]` for multiple resources, and more. A small degree of additional templating -- if you have the time and skill for it -- can also make this example even more generic and scale it far beyond what you have written in your example. And boost::out_ptr would require no additional changes to its usage to adapt. The rest of your review comments have already been addressed in the feature/boost.review branch and have been answered in other out_ptr threads. This API has been used with libavformat, Python's C API, OpenGL, DirectX, COM interfaces, and more. Previous iterations of this class have been used in VMWare driver infrastructure, all of Microsoft WRL library (as an improvement over its original CComPtr), and more (see the proposal p1132 <https://thephd.github.io/vendor/future_cxx/papers/d1132.html#motivation>, which details previous iterations of this). It has also been written by Raymond Chen, in his pursuit for a cleaner interface, but with some issues regarding destructor order for temporaries in function calls in C++ (which were correctly called out in the paper and the documentation before he wrote the article). See: https://devblogs.microsoft.com/oldnewthing/20190429-00/?p=102456 Finally, I have been vending this on my own time at no cost to individuals and companies for over a year, starting from its earlier versions from a (now superseded) repository: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/8MQhnL9rXBI/FaK1R.... I was also not the one to make this idea in the first place: I was taught the technique over 4 years ago by Mark Boyall in a C++ chat room. While I appreciate your criticisms, I do not think your examples and suggestions scale to Fortune 500 company code base infrastructure or small individual hobby codebases. out_ptr, however, does and has, and should be included in the Standard. And maybe Boost, too. Thank you for your time. Sincerely, JeanHeyd Meneide

Dear Andrzej Krzemienski, Scaling, in this terms, means with developer time. More comments below: I must admit, I do not understand this argumentation. No destructor to

write, but instead a deleter to write. At least destructor is in one place with the constructor.

Of course, if somebody later comes in, and starts adding members and functionality to this class it will get broked. C++ does not prevent users from conciously dong nasty things. But the point of such class would be to only manage a single resource. With this single responsibility in mind no-one will be adding function calls to the class to "improve" it.

There isn't any special code required to get to a nasty state: glGenBuffers sets errors that you must check with glGetError(). There's no way to communicate the return of glGetError() out of the destructor without either out parameters (std::file_system style) or throwing exceptions. The former is not the greatest design choice: the latter results in exactly the problems I defined with Phil Endecott's approach. out_ptr does not make throwing exceptions undesirable or require manual cleanup in the constructor: the cleanup goes once inside of the unique_ptr, once inside of the deleter, and never needs to be wrapped or written again.

...

There are similar problems with your PNG example.

All of this goes to show that while you can write cute one-off examples that seem simpler, they do not scale to the fundamental reality that is handing resources, dealing with exceptions, and more in C++ APIs.

What "scaling" do you mean here? I have seen one example of changing from managing one resource to managing a list of resources, which does not fit into my definition of "scaling". Building classes that have a single responsibility of managing a single resource does address the problem of dealing with exceptions and early returns. Also, I would like to know what you mean by "and more" in this claim.

The problem with the "wrap it in a class" or "write my own wrapper functions" is two-fold: it only works for that one bit of wrapped functionality, especially if the parameters are hardcoded. It becomes hard to extend and expose that functionality, while still maintaining exception safety and proper RAII. It is also fundamentally impossible to scale said wrappers: even if you manage to make a good wrapper for OpenGL, the structure of another library behaves rather differently. Even if the original example, we saw 2 different C libraries which handled their structure generation and error handling differently: one uses a GetLastError style of handling, and the other (libpng) has function pointers for errors. Writing a wrapper or transforming that into a class with a single responsibility that encapsulates all of the safety and necessary speed without leaking in the presence of exceptions or longjmp/setjmp error buffer handling or callbacks is difficult. out_ptr is successful because it does not try to wrap everything: it focuses on doing one thing -- safety of initialized values -- very well. That's why multiple companies with large C legacies have used it with the already enormous success that are smart pointers. And that's why other companies and individuals who have read the paper and seen it during the reviews have been very supportive of the proposal and the implementation.

Do you mean by "scaling" changing for m propagating a single resource to propagting a list of resources? How often do you do that? (Given that you have changed the type and now you have to change every place that uses the code.)

I should have been specific: I mean scaling with developer time. It is infinitely easier to reuse smart pointers designed for the task of resource management and making them play nice with existing C APIs then wrapping every single C API call you need to deal with, OpenGL or DirectX or Python C API or otherwise. This is not a conjecture: I spent 2 years wrapping OpenGL and DirectX APIs by-hand to create the perfect C++ interface. But it was brittle, it didn't apply to other C APIs which chose different mechanisms of error handling or resource allocation, and further problems. Instead of wrapping, I instead focused on safety and simply making the C API easier to use after Mark Boyall taught me about out_ptr. This resulted in much of my time spent doing what I was supposed to be doing -- graphics and simulations -- rather than meticulously picking at every API and trying to wring a good C++ object or interface out of it. In short: gentle, non-intrusive abstractions provided greater velocity for focusing effort where it mattered, and not having to piddle with details or accidentally leak resources when trying to use the default C++ error handling mechanism that is exceptions.

A small degree of additional templating

Well, "additional templating" doesn't sound as something good.

-- if

...

you have the time and skill for it --

What skills are required here?

Skill as in architecting a solution that works further beyond what I have shown here, keeping smart pointers to do the job of handling lifetime and having easy-to-use deleters which wrap additional parameters (for example, the size passed to a glGenBuffers call). I have written such systems before and they performed and worked well, but I think building such a thing in the middle of this mailing list might be a lot more effort than it is worth. That's not to say other people are skill-less: just trying to make such a layer as reusable and fast as possible takes some time and effort that's not exactly trivial but pays dividends in code reuse. All my best, JeanHeyd

Hi, I don't have time to comment further, and others seem to be expressing much the same thoughts that I have about this proposal, but there is one thing I have to mention: JeanHeyd Meneide wrote:

The rest of your review comments have already been addressed in the feature/boost.review branch and have been answered in other out_ptr threads.

Oh great, so this is another review where what we're supposed to be reviewing is a moving target? That's frustrating. I've not followed much of the discussion, really due to lack of time but I'll also claim that it allows me to form my own opinions without undue influence from what other people think - and now having read the documentation linked from the review announcement I hear that I should have been looking at another branch. Apart from the extra work that this causes for reviewers, it also is in danger of leading to "design by review". Rapidly re-designing something based on a jumble of feedback from multiple people in a short time is not a good way to produce the best solution to a problem, IMHO. Review managers (etc.), in future can we please just review what is submitted? Regards, Phil.