
Thanks to JeanHeyd for going through the Boost review process, and to Zach for managing the review. Briefly: I do not have enough reasons to recommend acceptance for out_ptr just yet --- in its current form. (I'd like to see it return for a new review when the feedback from this review shapes the design so that some of these concerns are resolved). Second: This excludes "inout_ptr" (for which I had even more concerns) and just focusing on "out_ptr" - which is the simpler and more common use case of the two. Below comparing out_ptr vs the hand written code that a user would write to use a /typical/ C out-pointer API with smart pointers, this is what I'm comparing: [Typical code user would write]: Type* raw; Result result = TheCApi(args..., &raw); if (!Success(result)) { // failure (e.g. return, or branch and handle error) } Smart<Type, Deleter> smart(raw, etc...); [What happens with out_ptr now]: /* 'smart' already has to exist here before TheCApi is called. Either as Smart<Type, Deleter> smart; or Smart<Type, Deleter> smart(nullptr, etc...); depending on deleter */ /* User writes: Result result = TheCApi(args..., out_ptr(smart)); // or Result result = TheCApi(args..., out_ptr(smart, etc...)); depending on deleter */ Type* raw = nullptr; Result result = TheCApi(args..., &raw); if (raw != nullptr) { smart.reset(raw); // or smart.reset(raw, etc...); depending on deleter } if (!Success(result)) { // failure (e.g. return, or branch and handle error) } Concern 1. Having to materialize the smart pointer before TheCApi is called. Ordinarily this might be fine for some smart pointers and stateful deleters, but it may not be for others. Concern 2. The original user code initializes the smart pointer based on Success(result). The out_ptr version would do so based on whether the raw out pointer was changed to something non-null by TheCApi. This might be fine for some cases, but it may not be for others. That it isn't the same as fundamental logic as the user code bothers me, though. Concern 3. The equivalent of the hand written user code is less overhead. Personally I'm of the opinion that maybe this doesn't matter too, but here the author does say he has users who cannot abide by it, and that this facility needs support from e.g. std::unique_ptr to be viable. (And this leads to concern 4 below). [I'm not a fan of needing unique_ptr implementations to add support for this to make it comparable to some user's hand written code]. Concern 4. The UB optimization that the author wrote because of Concern 3 is a real concern to him. The UB optimization has no place in Boost, and one of which doesn't even work with many C++ library vendors' unique_ptr. [Fortunately the Boost reviews have already echoed this sentiment and I believe this is going away]. Concern 5. Supporting other custom smart pointers requires effectively re-implementing out_ptr_t by specialization. I have come to see this as not viable. [Already mentioned in other reviews] Concern 6. C APIs where the deleter (its state, logic, etc.) are governed by some other result from TheCApi. This is can be worked around in many cases, but the workaround is clumsy. Or insupportable, in other cases. i.e. The stateful deleter is always initialized with 'etc...' either before-hand via Smart<Type> smart(nullptr, etc...) or at call site with: TheCApi(args, out_ptr(etc...)); before analyzing other results of calling TheCApi. (The 7th concern I had made notes about pre-review has already been eliminated, when the unconditionally noexcept destructor is now only conditionally noexcept). Other thoughts: The suggestion by Emil lead to an interesting alternative idea to this problem domain, but I had no time to fully explore it. I don't think it would address all of these concerns. It has some other greater appeal, e.g. a way to call C APIs that materializes the smart pointers as results (along with C API return value) of an "Invoke-style" thing. Or even a way to transform a C API with out-pointers into a C++ callable that returns smart pointers (along with C API return value) via a "Bind-style" thing. Glen
participants (1)
-
Glen Fernandes