Discussion Thread: DIP 1040--Copying, Moving, and Forwarding--Community Review Round 1

[tsbockman]

On Thursday, 11 March 2021 at 08:36:18 UTC, Walter Bright wrote:
> On 3/10/2021 8:17 PM, tsbockman wrote:
>> Wouldn't it make more sense to just skip the move operation 
>> entirely when the source and destination are the same? Or are 
>> there circumstances in which that cannot be determined, even 
>> at runtime?
>
> The idea is that the move assignment operation takes care of 
> this, and makes it "as if" the move was done, then the 
> destruction.

Is it the responsibility of the explicit body of the custom move 
assignment operator to do the destruction, or the responsibility 
of the compiler to insert a call to the destructor somewhere? If 
the compiler inserts the call, is this done inside the move 
assignment operator, or at each call site?

My concern is how to make code like this work correctly:

/////////////////////////////////////////////////
module app;

ptrdiff_t netAllocCount = 0;
void* malloc(size_t size) @system nothrow @nogc {
     import core.stdc.stdlib : malloc;
     void* ptr = malloc(size);
     netAllocCount += int(ptr !is null);
     return ptr;
}
void free(void* ptr) @system nothrow @nogc {
     import core.stdc.stdlib : free;
     netAllocCount -= int(ptr !is null);
     free(ptr);
}

struct S {
private:
     struct Data {
         uint data;
         int refCount = 1;
     }

     Data* ptr;
     Data internal;

public:
     bool isUnique() const pure @safe nothrow @nogc {
         return (ptr is &internal); }
     ref inout(uint) data() return inout pure @safe nothrow @nogc {
         return ptr.data; }

     // normal construction and assignment
     this(bool unique) @trusted nothrow @nogc {
         construct(unique); }
     private void construct(bool unique) @trusted nothrow @nogc {
         pragma(inline, false); // Don't let inlining make things 
work by accident.
         if(unique) {
             ptr = &internal;
         	internal = Data.init;
         } else {
             ptr = cast(Data*) malloc(size_t.sizeof * 2);
             (*ptr) = Data.init;
         }
     }
     ref typeof(this) opAssign(bool unique) return @safe nothrow 
@nogc {
         destruct(this);
         construct(unique);
         return this;
     }

     // copy construction
     @disable this(this);
     this(ref typeof(this) source) pure @trusted nothrow @nogc {
         if(source.isUnique) {
             ptr = &internal;
             internal = source.internal;
         } else {
             ptr = source.ptr;
             ptr.refCount += 1;
         }
     }

     // move construction and assignment (using the DIP's 
misleading syntax):
     this(S source) pure @trusted nothrow @nogc {
         if(source.isUnique) { // This works, because source is 
really by reference.
             ptr = &internal;
             internal = source.internal;
         } else
             ptr = source.ptr;
     }
     void opAssign(S source) @trusted nothrow @nogc {
         // What goes here, and what is the full lowering of a 
move assignment call?
     }

     // destructor
     ~this() @safe nothrow @nogc {
         destruct(this); }
     private void destruct(ref S s) @trusted nothrow @nogc {
         pragma(inline, false); // Don't let inlining make things 
work by accident.
         if(!s.isUnique) {
             s.ptr.refCount -= 1;
             if(s.ptr.refCount <= 0)
                 free(s.ptr);
         }
         s.ptr = null;
     }
}

void main() @safe {
     import std.stdio : writeln;
     {
         S a = true, b = false, c = b;
         a.data = 1;
         b.data = 2;
         a = b;
         c.data = 3;
         assert(a.data == 3);
     }
     assert(netAllocCount == 0);
}
/////////////////////////////////////////////////

Syntax aside, I think the way it should be done is this:

// Lower the `a = b;` line in main, above, to this:
if(&b !is &a) { // Moving a variable into itself is a no-op.
     destroy(a);
     /* Just call the move constructor; there is no point
     to the move assignment operator as its body would be 
identical: */
     a.__ctor(b);
}

I've tested this algorithm, and it seems to work as intended 
(although of course it has to be expressed somewhat differently 
without the DIP). But, that's clearly not what you have in mind. 
I just can't figure out what you think the alternative is:

// Maybe this is your proposed lowering for the `a = b;` line in 
main, above?
{
     S oldDest = a; // You all say no copy constructor call is 
necessary here, but...
     a.opAssign(b); // (Body is the same as that of the move 
constructor.)
     // ...the implicit destroy(oldDest); at scope exit crashes if 
oldDest was only blit.
}

You can put the lowering logic inside of the move opAssign 
instead, but it doesn't change the fundamental problem. If no 
actual general-purpose lowering is possible that accurately 
reflects the intended semantics of, "After the move is complete, 
the destructor is called on the original contents of the 
constructed object," then the DIP's description of the semantics 
is simply incorrect and should be replaced with something more 
rigorous.