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