March 12, 2021
On Friday, 12 March 2021 at 11:35:03 UTC, Imperatorn wrote:

>
> The discussion implies there are parts not fully understood and that makes people nervous.

That's why we're here. The point of the DIP review process is to improve the DIP. Does it need more examples? Does it need more clarity? Did the author miss something important? Discussion threads go in all sorts of directions. Ultimately, the author will decide whether or not changes are warranted based on the discussion here and the comments in the Feedback Thread.

Very rare is the DIP that gets through the process without revision.



March 12, 2021
On Friday, 12 March 2021 at 02:06:19 UTC, Walter Bright wrote:
> On 3/11/2021 4:57 PM, deadalnix wrote:
>> These problems seems to arise due to the fact postblit did not distinguish between move and copy.
>> 
>> postblit is inappropriate for copy, but as far as I can tell, not only does it work for move, but it's the only path that do not involve creating yet more magic that's going to bite us in the ass at some point.
>
> postblit bit us in the ass quite a bit. (postblit didn't do moves)

You are making my point, yet for some reason miss it anyways.

Postblit works with *1* object. It fails at being a good copy mechanism because copying involves *2* objects, by definition.

Move works with *1* object. opAssign will, similarly to what happened when using postblit for copies, lead to problems because it inherently works with *2* objects.

Postblit is a natural fit for moves because both work with *1* object, thus not leaving an object out there is some sort of magic state that we can never figure out what to do with.

There is already a fair bit of magic that is proposed to be added to opAssign in this proposal, yet it is now obvious to me that there are already holes in it. For instance, if one of the fields of the object ends up not being moved, how will this fields ends up being destroyed? How is that even fixable considering the whole thing can be n-levels deep? The only way I this being fixed is be reintroducing the notion that structs must have a null state and destroy the object anyways post move, but going there would be a big optimization barrier, while not going there creates a situation where things that should be destroyed ends up not being, which is also pretty bad.

On the other hand, the postblit by its very nature does not allow for this whole situation to arise to begin with. If a field is not going to be moved, it will have to see some new value assigned there, causing the destruction of the old value.
March 12, 2021
On Friday, 12 March 2021 at 10:07:41 UTC, Walter Bright wrote:
> On 3/11/2021 4:53 PM, deadalnix wrote:
>> On Thursday, 11 March 2021 at 21:41:59 UTC, Walter Bright wrote:
>>> Why? There can be no other uses of the rvalue, so why not simply move it?
>> 
>> If you have two objects in your move constructor, but only one at the end, then a destruction must take place somewhere.
>
> The whole point of move construction is to move an initialized object to an unconstructed object. No destruction is needed.
>
> It's move assignment that needs to destruct something (the original value of the destination).

How do you ensures that there aren't any leftover that need to be destroyed or enforce that there are no leftovers? Enforcing it seems like an impossible task to be, or at least extremely complex. So that means you need to call the destructor anyways and that forces the struct to have a null state.
March 12, 2021
On Friday, 12 March 2021 at 11:47:49 UTC, Max Haughton wrote:
> On Friday, 12 March 2021 at 11:35:03 this isn't the place to be talking about pedagogy

So what's the proper place?
March 12, 2021
On Friday, 12 March 2021 at 12:18:56 UTC, Mike Parker wrote:
> On Friday, 12 March 2021 at 11:35:03 UTC, Imperatorn wrote:
>
>>
>> The discussion implies there are parts not fully understood and that makes people nervous.
>
> That's why we're here. The point of the DIP review process is to improve the DIP. Does it need more examples? Does it need more clarity? Did the author miss something important? Discussion threads go in all sorts of directions. Ultimately, the author will decide whether or not changes are warranted based on the discussion here and the comments in the Feedback Thread.
>
> Very rare is the DIP that gets through the process without revision.

👍
March 12, 2021
On Wednesday, 10 March 2021 at 22:51:58 UTC, tsbockman wrote:
> On Friday, 5 March 2021 at 23:03:57 UTC, tsbockman wrote:
>> On Friday, 5 March 2021 at 12:19:54 UTC, Mike Parker wrote:
>>> [...]
>>
>> From the DIP:
>>>     [...]
>>
>> Is the parameter to these methods really pass-by-value?
>> ...
>> If the parameter is, in fact, intended to be pass-by-reference, then I must strenuously object to the chosen syntax.
>
> Over in the feedback thread, Atila Neves also concluded that the syntax is misleading here:
>
> On Wednesday, 10 March 2021 at 21:27:25 UTC, Atila Neves wrote:
>> I eventually understood what this meant, but this confused me when I read it the first time. I'd reword it to mention that the syntax looks like a by-value parameter but ends up being passed by reference. It also confused me that the 2nd function had `ref` in there.

I didn't object to the syntax, especially since it's the same syntax used right now for moves. I got confused by the explanation, since it mixes syntax with the underlying mechanics.
March 12, 2021
On 3/11/2021 6:06 PM, Walter Bright wrote:
> postblit bit us in the ass quite a bit. (postblit didn't do moves)

Ack. Of course it did moves.
March 12, 2021
On Friday, 12 March 2021 at 19:50:57 UTC, Atila Neves wrote:
> On Wednesday, 10 March 2021 at 22:51:58 UTC, tsbockman wrote:
>> Over in the feedback thread, Atila Neves also concluded that the syntax is misleading here:
>>
>> On Wednesday, 10 March 2021 at 21:27:25 UTC, Atila Neves wrote:
>>> I eventually understood what this meant, but this confused me when I read it the first time. I'd reword it to mention that the syntax looks like a by-value parameter but ends up being passed by reference. It also confused me that the 2nd function had `ref` in there.
>
> I didn't object to the syntax,

I didn't mean to imply that you objected, just that you confirmed that by-value parameter syntax is being used where the parameter is actually semantically by-reference.

> especially since it's the same syntax used right now for moves.

No, it's not. You are conflating "moves" with "custom move operators definitions", a proposed implementation detail of the lowering for actual moves.

This is the syntax for moves and copies, depending on the surrounding context:

    B b = a;
    C c = b;
    return f(c);

The closest thing we have to a dedicated move syntax is this:

    import std.algorithm.mutation : move;
    move(a, b);

As for the proposed move operator syntax - no, we do not currently use that syntax for move operators. None of the various methods of moving things call them:

//////////////////////////////
struct S {
    int x;
    this(int x) @safe {
        this.x = x; }
    this(S s) @safe {
        x = s.x * 2; }
    void opAssign(S s) @safe {
        x = -s.x; }
}

S g(S s) {
    return s; }

void main() @system {
    import std.stdio : writeln;
    import std.algorithm.mutation : move;

    S s = 1, t = s, u;
    writeln(t);
    move(t, u);
    writeln(u);
    writeln(g(u));
}
//////////////////////////////

Output:

    S(1)
    S(1)
    S(1)

Even if I manually call S.opAssign, a copy is performed for the parameter, so it's *really* not a move operator. We don't have custom move operators at all right now; that's the point of this DIP, isn't it?
March 12, 2021
On Friday, 12 March 2021 at 22:46:05 UTC, tsbockman wrote:
> As for the proposed move operator syntax - no, we do not currently use that syntax for move operators. None of the various methods of moving things call them:
> ...
> Even if I manually call S.opAssign, a copy is performed for the parameter, so it's *really* not a move operator.

I accidentally left out assignment in my example, but that's OK because it requires a different example to demonstrate the copy problem:

////////////////////////////////////
struct S {
    int x;
    this(int x) @safe {
        this.x = x; }
    this(ref typeof(this) s) @safe {
        x = 0; }
    void opAssign(S s) @safe {
        x = -s.x; }
}

void main() @system {
    import std.stdio : writeln;
    import std.algorithm.mutation : move;

    S s = 1, t;
    t = s;
    writeln(t);
}
////////////////////////////////////

Output:

    S(0)

So, the opAssign does get called in that case, but it's not a move because the copy constructor gets called first.
March 12, 2021
On Friday, 5 March 2021 at 12:19:54 UTC, Mike Parker wrote:
> This is the discussion thread for the first round of Community Review of DIP 1040, "Copying, Moving, and Forwarding":

The default field-wise move operators seem to have been specified with no concern for maintaining consistency between the semantics of custom operators and default operators:

From the DIP:
> If a Move Assignment Operator is not defined for a struct that
> has a Move Constructor, a default Move Assignment Operator is
> defined and implemented as a move for each of its fields, in
> lexical order.

If a custom move constructor is present for a good reason, that means that simply moving the fields one-by-one is *not* the desired move behavior.

Instead, the default move assignment operator should call the struct's move constructor and destructor in one of the two valid patterns that I found earlier in this discussion (link to a full working example):
    https://gist.github.com/run-dlang/b789714c01905f091a44ee2666276433

// Using an alternative syntax so that this can be tested today:
void moveAssign(ref S source) @trusted nothrow @nogc {
    static if(useDIPLowering) {
        // destroy after (the DIP's proposal):
        S newVal = void;
        newVal.moveConstruct(source);
        S oldVal = void;
        oldVal.moveConstruct(this);
        moveConstruct(newVal);
        // Implicitly destruct(oldVal).
    } else {
        // conditionally move and destroy before (my proposal):
        if(&source !is &this) {
            destruct(this);
            moveConstruct(source);
        }
    }
}

Also from the DIP:
> If a Move Constructor is not defined for a struct that has a Move
> Assignment Operator, a default Move Constructor is defined and
> implemented as a move for each of its fields, in lexical order.

Again, the presence of a custom move assignment operator indicates that non-default move behavior is desired. Auto-generating a default move constructor is a missed opportunity to point out a semantic inconsistency in the user's work.

Defining a custom move assignment operator without a custom move constructor to go with it should be a compile-time error, because there is no automated way to extract a valid move constructor from a custom move assignment operator.

In the unlikely case that the omission of the custom move constructor is intentional, people can manually defining a field-wise move constructor (which is not difficult), or annotate that constructor with @disable.