www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Built-in array opSliceAssign

reply Eduard Staniloiu <edi33416 gmail.com> writes:
Hello, everyone!

I have a question regarding the expected behaviour of the 
built-in array's opSliceAssign.

Given the following code:

```
import std.stdio;

struct A
{
     int x;

     ref A opAssign(A rhs)
     {
         writefln("slice_bug.opAssign: begin");

         return this;
     }
}

void main(string[] args)
{
     A[] a = [A(1), A(2), A(3)];
     A[] b = [A(2), A(3), A(4)];

     // Expecting opAssign to be called for every element in a
     a[] = b[];

     // In other words, I was under the impression that the above
     // is sintactic-sugar for
     for (size_t i = 0; i < a.lenght; ++i)
     {
         a[i] = b[i]; // This calls opAssign, as expected
     }
}
```

As I wrote in the comments above, I was expecting `a[] = b[]` to 
iterate the slices and assign the elements of b into a.

What really happens is a memcpy: as you can see from godblot [0], 
this gets lowered to a call to `_d_arraycopy`, in druntime.

I'm not sure if this is the intended behaviour, though.
I'm saying this as I've got bitten by this, because I'm doing 
reference counting inside opAssign.

IMHO, this is a bug. The code should lower to calls to opAssing 
for types that define opAssign.

I've also pasted the code on https://run.dlang.io/is/vneELO

[0] - https://godbolt.org/z/_IXCAV
Oct 25 2018
next sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Thursday, 25 October 2018 at 12:25:37 UTC, Eduard Staniloiu 
wrote:

 As I wrote in the comments above, I was expecting `a[] = b[]` 
 to iterate the slices and assign the elements of b into a.

 What really happens is a memcpy: as you can see from godblot 
 [0], this gets lowered to a call to `_d_arraycopy`, in druntime.


In D, when you assign one aggregate to another, opAssign is only 
called for the aggregate, not for any of its elements. However, 
postblit constructors are called for both. Example: 
https://run.dlang.io/is/XfDaWw

Your example will work as expected if you change the opAssign to 
a postblit constructor: https://run.dlang.io/is/HBbGO2
Oct 25 2018
parent reply Eduard Staniloiu <edi33416 gmail.com> writes:
On Thursday, 25 October 2018 at 12:38:44 UTC, Paul Backus wrote:

 On Thursday, 25 October 2018 at 12:25:37 UTC, Eduard Staniloiu 
 wrote:

 As I wrote in the comments above, I was expecting `a[] = b[]` 
 to iterate the slices and assign the elements of b into a.

 What really happens is a memcpy: as you can see from godblot 
 [0], this gets lowered to a call to `_d_arraycopy`, in 
 druntime.


 In D, when you assign one aggregate to another, opAssign is 
 only called for the aggregate, not for any of its elements. 
 However, postblit constructors are called for both. Example: 
 https://run.dlang.io/is/XfDaWw


Can you, please, give me a link to where it says this in the 
specs?

Based on the example, I would expect that the code gets lowered 
to some version of `_d_arrayassign` [0]. I still think that this 
is problematic, as it's unexpected to the user: you're expecting 
the assignment operator to be called, not the postblit.

I know that the compiler can and will create an opAssign if a 
postblit or dtor is defined, as you can write the assignment as a 
this._dtor; blit rhs into this.

This being said, I think that if the user took the time to define 
opAssign, it should be called, because he might want to do 
something extra when an assignment occurs: an ex. having 
different logs "creating new obj" vs "changing existing obj".



 Your example will work as expected if you change the opAssign 
 to a postblit constructor: https://run.dlang.io/is/HBbGO2
Oct 25 2018
parent Eduard Staniloiu <edi33416 gmail.com> writes:
On Thursday, 25 October 2018 at 13:01:06 UTC, Eduard Staniloiu 
wrote:

 On Thursday, 25 October 2018 at 12:38:44 UTC, Paul Backus wrote:

 On Thursday, 25 October 2018 at 12:25:37 UTC, Eduard Staniloiu 
 wrote:

 As I wrote in the comments above, I was expecting `a[] = b[]` 
 to iterate the slices and assign the elements of b into a.

 What really happens is a memcpy: as you can see from godblot 
 [0], this gets lowered to a call to `_d_arraycopy`, in 
 druntime.


 In D, when you assign one aggregate to another, opAssign is 
 only called for the aggregate, not for any of its elements. 
 However, postblit constructors are called for both. Example: 
 https://run.dlang.io/is/XfDaWw


 Can you, please, give me a link to where it says this in the 
 specs?

 Based on the example, I would expect that the code gets lowered 
 to some version of `_d_arrayassign` [0]. I still think that 
 this is problematic, as it's unexpected to the user: you're 
 expecting the assignment operator to be called, not the 
 postblit.

 I know that the compiler can and will create an opAssign if a 
 postblit or dtor is defined, as you can write the assignment as 
 a this._dtor; blit rhs into this.

 This being said, I think that if the user took the time to 
 define opAssign, it should be called, because he might want to 
 do something extra when an assignment occurs: an ex. having 
 different logs "creating new obj" vs "changing existing obj".



 Your example will work as expected if you change the opAssign 
 to a postblit constructor: https://run.dlang.io/is/HBbGO2




Accidentally sent to early.

One extra reason as to why, imho, this implicit call to the 
postblit is evil, is that a lot of code will probably break when 
we'll transition to the CopyConstructor. See RazvanN's PR [0].

This is actually how I stumbled upon this, as I am using his 
branch with my repo.

[0] - https://github.com/dlang/dmd/pull/8688
Oct 25 2018
prev sibling parent reply Adam D. Ruppe <destructionator gmail.com> writes:
On Thursday, 25 October 2018 at 12:25:37 UTC, Eduard Staniloiu 
wrote:

 IMHO, this is a bug. The code should lower to calls to opAssing 
 for types that define opAssign.


The spec doesn't exactly say it uses memset, but it does imply it:

https://dlang.org/spec/arrays.html#array-copying

talking about "aggressive parallel code optimizations than 
possible with the serial semantics of C" and "copying" rather 
than "assigned" etc.

but indeed postblit lets this work.
Oct 25 2018
parent reply Eduard Staniloiu <edi33416 gmail.com> writes:
On Thursday, 25 October 2018 at 12:55:38 UTC, Adam D. Ruppe wrote:

 On Thursday, 25 October 2018 at 12:25:37 UTC, Eduard Staniloiu 
 wrote:

 IMHO, this is a bug. The code should lower to calls to 
 opAssing for types that define opAssign.


 The spec doesn't exactly say it uses memset, but it does imply 
 it:

 https://dlang.org/spec/arrays.html#array-copying

 talking about "aggressive parallel code optimizations than 
 possible with the serial semantics of C" and "copying" rather 
 than "assigned" etc.

 but indeed postblit lets this work.


You are right. Thank you!

I guess I never read/understood it like this.
I expected it to use opAssign as that is what's the most natural 
and intuitive decision for me.

I take it that this is the expected behaviour, then.
Oct 25 2018
parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Thursday, 25 October 2018 at 13:22:36 UTC, Eduard Staniloiu 
wrote:


 The spec doesn't exactly say it uses memset, but it does imply 
 it:

 https://dlang.org/spec/arrays.html#array-copying

 talking about "aggressive parallel code optimizations than 
 possible with the serial semantics of C" and "copying" rather 
 than "assigned" etc.

 but indeed postblit lets this work.


 You are right. Thank you!

 I guess I never read/understood it like this.
 I expected it to use opAssign as that is what's the most 
 natural and intuitive decision for me.

 I take it that this is the expected behaviour, then.


I don't think interpreting the spec like that is correct. It says 
"...it means that the *contents* of the array are the target of 
the *assignment*...", and further, in the examples:

s[1..2] = t[0..1]; // same as s[1] = t[0]
s[0..2] = t[1..3]; // same as s[0] = t[1], s[1] = t[2]

The current behavior of the compiler is quite the opposite of 
those "same as" above.

Consider:

struct S {
      disable this(this); // can't implicitly copy, i.e. pass 
lvalue to a function
     void opAssign(ref S rhs) { /* ... */ } // but can explicitly 
copy
}

void main() {
     S x, y;
     x = y; // works
     S[10] a;
     a[0 .. 3] = y;         // doesn't compile, i.e. NOT "same as 
a[0] = y, a[1] = y, a[2] = y"
     a[0 .. 3] = a[3 .. 6]; // doesn't compile either, i.e. NOT 
"same as a[0] = a[3], a[1] = a[4], a[2] = a[5]"
}

I've filed an issue about this around a month ago: 
https://issues.dlang.org/show_bug.cgi?id=19274
Oct 25 2018
parent reply Adam D. Ruppe <destructionator gmail.com> writes:
On Thursday, 25 October 2018 at 19:56:18 UTC, Stanislav Blinov 
wrote:

 The current behavior of the compiler is quite the opposite of 
 those "same as" above.


Yeah, I guess I am maybe selectively reading the spec in light of 
the implementation... but I think the examples are just sloppy. 
Or maybe we have a buggy implementation but idk which is more 
useful.
Oct 25 2018
parent Eduard Staniloiu <edi33416 gmail.com> writes:
On Thursday, 25 October 2018 at 21:00:46 UTC, Adam D. Ruppe wrote:

 On Thursday, 25 October 2018 at 19:56:18 UTC, Stanislav Blinov 
 wrote:

 The current behavior of the compiler is quite the opposite of 
 those "same as" above.


 Yeah, I guess I am maybe selectively reading the spec in light 
 of the implementation... but I think the examples are just 
 sloppy. Or maybe we have a buggy implementation but idk which 
 is more useful.


I still hold my believe that if opAssign is defined then that 
should be used.

In my humble opinion, the current way might/could be faster, but 
it's not correct.
Oct 30 2018