Re: out_ptr's formal review starts now!

From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2019-06-27 07:56:21

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#poorly-designed-c-apis 
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.