Bug 7444 – Require [] for array copies too

Status
REOPENED
Severity
enhancement
Priority
P4
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2012-02-05T11:10:57Z
Last change time
2024-12-13T17:58:18Z
Keywords
pull
Assigned to
No Owner
Creator
bearophile_hugs
Moved to GitHub: dmd#18411 →

Comments

Comment #0 by bearophile_hugs — 2012-02-05T11:10:57Z
This is derived from issue 3971 see there for more (messy) discussions. In 2.058 this compiles: int[100] foo() { int[100] a; return a; } void main() { int[10_000] a, b; auto c = new int[10_000]; a = 1; a = b; a = c; auto d = foo(); } But for consistency of the vector ops syntax, and to help the programmer spot where the code is doing a potentially long copy, I suggest to turn that first program into a compile-time error, and require [] on lvalues everywhere you perform a vector operation, like when you copy a int[N] on another int[N], etc: int[100] foo() { int[100] a; return a; } void main() { int[10_000] a, b; auto c = new int[10_000]; a[] = 1; a[] = b; a[] = c; auto d[] = foo(); // currently an error } - - - - - - - - - - - - - - - - But note that normally all array ops require a [] even on the rvalue: void main() { int[10_000] a, b; a[] += b[]; } So an alternative stricter proposal is to require [] on the right too: int[100] foo() { int[100] a; return a; } void main() { int[10_000] a, b; auto c = new int[10_000]; auto d = new int[10_000]; a[] = 1; a[] = b[]; a[] = c[]; c[] = d[]; auto e[] = foo()[]; // currently an error } It helps the programmer tell apart two different cases, that currently are usable with the same syntax (from an example by Don): void main() { int[][3] x; int[] y; int[] z; x[] = z; // copies just the z pointer y[] = z; // copies the elements in z } Requiring [] on the right (rvalue) for the copy of many items avoids the ambiguity: void main() { int[][3] x; int[] y; int[] z; x[] = z; y[] = z[]; }
Comment #1 by timon.gehr — 2012-02-05T15:34:04Z
While I agree that the syntax should be enforced more strictly in general, I still completely disagree with requiring [] on static array copies. Static arrays are value types of fixed size, and should be treated as such. Requiring [] is just wrong. void foo(int[4] x){} void foo(int[] y){} void main(){ int[4] x, y; struct S{int[4] x;} S a, b; x = y; a = b; // why should this work if the above does not? foo(x); // copies, you want this to be an error foo(x[]); // calls the other overload, does not copy }
Comment #2 by k.hara.pg — 2012-02-09T05:33:52Z
(In reply to comment #1) > While I agree that the syntax should be enforced more strictly in general, I > still completely disagree with requiring [] on static array copies. Static > arrays are value types of fixed size, and should be treated as such. Requiring > [] is just wrong. > > void foo(int[4] x){} > void foo(int[] y){} > > void main(){ > int[4] x, y; > struct S{int[4] x;} > S a, b; > x = y; > a = b; // why should this work if the above does not? > foo(x); // copies, you want this to be an error > foo(x[]); // calls the other overload, does not copy > } I completely agree with Timon. When x and y are declared with same type T, x = y is identity assignment, and it is valid syntax even if T is static array type.
Comment #3 by k.hara.pg — 2012-02-09T06:31:34Z
I'd like to provide an exhaustive test that should compile after fixing the enhancement suggested by me and timon. ---- // X: Changed accepts-invalid to rejects-invalid by this issue // a: slice assginment // b: element-wise assignment static assert(!__traits(compiles, { sa = e; })); // X static assert( __traits(compiles, { sa[] = e; })); // b static assert(!__traits(compiles, { da = e; })); static assert( __traits(compiles, { da[] = e; })); // b // lhs is static array static assert( __traits(compiles, { sa = sa; })); // b == identity assign static assert(!__traits(compiles, { sa = sa[]; })); // X static assert(!__traits(compiles, { sa[] = sa; })); // X static assert( __traits(compiles, { sa[] = sa[]; })); // b static assert(!__traits(compiles, { sa = da; })); // X static assert(!__traits(compiles, { sa = da[]; })); // X static assert( __traits(compiles, { sa[] = da; })); // b static assert( __traits(compiles, { sa[] = da[]; })); // b // lhs is dynamic array static assert(!__traits(compiles, { da = sa; })); // X static assert( __traits(compiles, { da = sa[]; })); // a static assert(!__traits(compiles, { da[] = sa; })); // X static assert( __traits(compiles, { da[] = sa[]; })); // b static assert( __traits(compiles, { da = da; })); // a == identity assign static assert( __traits(compiles, { da = da[]; })); // a static assert( __traits(compiles, { da[] = da; })); // b static assert( __traits(compiles, { da[] = da[]; })); // b ---- bearophile says that sa = sa should be rejected and must replace it to sa[] = sa[]. But I and timon disagree it.
Comment #4 by timon.gehr — 2012-02-09T07:01:17Z
Maybe sa[] = da and da[] = da should be '// X' too.
Comment #5 by k.hara.pg — 2012-02-10T04:23:32Z
(In reply to comment #4) > Maybe sa[] = da and da[] = da should be '// X' too. Hmm, OK. If it is an element-wise assignment, we should add explicit slicing both side of AssignExp. I've posted a pull to implement this enhancement: https://github.com/D-Programming-Language/dmd/pull/702 Updated test: https://github.com/D-Programming-Language/dmd/pull/702/files#L6R250
Comment #6 by bugzilla — 2012-02-17T23:27:14Z
The enhancement is to disallow: sa = da; // use sa[] = da[] instead sa = e; // use sa[] = e instead I'm not sure this is worth the existing code breakage.
Comment #7 by bearophile_hugs — 2012-02-18T05:02:07Z
(In reply to comment #6) > I'm not sure this is worth the existing code breakage. I thin it's worth it, if you want with warning / deprecation / error stages. But if want, I'll ask for a little voting in the main D newsgroup.
Comment #8 by bearophile_hugs — 2012-02-22T16:07:54Z
See why a consistent syntax matters: Issue 7564
Comment #9 by github-bugzilla — 2012-10-14T19:19:00Z
Commits pushed to master at https://github.com/D-Programming-Language/druntime https://github.com/D-Programming-Language/druntime/commit/a77c82772e7e7b2d1d863b1fb56b614b9d4bc6a1 fix Issue 7444 - Require [] for array copies too https://github.com/D-Programming-Language/druntime/commit/be3a7fa1bc726b453203c058ff2fa8c81dcfcab1 Merge pull request #314 from 9rnsr/fix7444 Supplemental changes for Issue 7444 - Require [] for array copies too
Comment #10 by github-bugzilla — 2012-11-20T09:55:06Z
Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/28dedee456e741f02f08a944f41daa9e46236224 Issue 7444 - Require [] for array copies too https://github.com/D-Programming-Language/phobos/commit/9a6dad8a2ac5841bdcfa2b86082450818f6eefab Merge pull request #960 from 9rnsr/fix7444 Supplemental changes for Issue 7444 - Require [] for array copies too
Comment #11 by github-bugzilla — 2012-11-21T20:12:48Z
Comment #12 by github-bugzilla — 2013-03-06T22:53:51Z
Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/57b770ed49379c5af726d23356e0f75818a3f859 Issue 7444 - Require [] for array copies too https://github.com/D-Programming-Language/dmd/commit/ba1009c5561b51b8f18d9c869fde9bd45cb7ebc7 Merge pull request #702 from 9rnsr/fix7444 Issue 7444 - Require [] for array copies too
Comment #13 by bearophile_hugs — 2013-03-07T04:22:27Z
(In reply to comment #12) > Commits pushed to master at https://github.com/D-Programming-Language/dmd > > https://github.com/D-Programming-Language/dmd/commit/57b770ed49379c5af726d23356e0f75818a3f859 > Issue 7444 - Require [] for array copies too > > https://github.com/D-Programming-Language/dmd/commit/ba1009c5561b51b8f18d9c869fde9bd45cb7ebc7 > Merge pull request #702 from 9rnsr/fix7444 > > Issue 7444 - Require [] for array copies too I have tried this change, and now the first test case of this ER: int[100] foo() { int[100] a; return a; } void main() { int[10_000] a, b; auto c = new int[10_000]; a = 1; a = b; a = c; auto d = foo(); } gives a ICE: temp.d(8): Warning: explicit element-wise assignment (a)[] = 1 is better than a = 1 temp.d(10): Warning: explicit element-wise assignment (a)[] = (c)[] is better than a = c Assertion failure: '0' on line 1193 in file 'glue.c'
Comment #14 by bearophile_hugs — 2013-03-07T04:58:19Z
(In reply to comment #13) > gives a ICE: Smaller test case: void main() { int[1] a; a = 1; }
Comment #15 by k.hara.pg — 2013-03-07T05:14:27Z
(In reply to comment #14) > (In reply to comment #13) > > > gives a ICE: > > Smaller test case: > > > void main() { > int[1] a; > a = 1; > } What version and compiler switch do you use? I cannot reproduce the ICE.
Comment #16 by bearophile_hugs — 2013-03-07T12:10:52Z
(In reply to comment #15) > What version and compiler switch do you use? I cannot reproduce the ICE. I am using the GIT head compiler, I have downloaded and compiled dmd few hours ago, after this patch was merged. I am on Windows 32 bit, and I have compiled the code with: dmd -wi test.d
Comment #17 by k.hara.pg — 2013-03-07T15:41:56Z
(In reply to comment #16) > (In reply to comment #15) > > > What version and compiler switch do you use? I cannot reproduce the ICE. > > I am using the GIT head compiler, I have downloaded and compiled dmd few hours > ago, after this patch was merged. > > I am on Windows 32 bit, and I have compiled the code with: > > dmd -wi test.d Thanks. -w option does not reproduce it, but -wi does.
Comment #18 by k.hara.pg — 2013-03-07T17:00:31Z
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > > > What version and compiler switch do you use? I cannot reproduce the ICE. > > > > I am using the GIT head compiler, I have downloaded and compiled dmd few hours > > ago, after this patch was merged. > > > > I am on Windows 32 bit, and I have compiled the code with: > > > > dmd -wi test.d > > Thanks. -w option does not reproduce it, but -wi does. OK, I opened a new report bug 9663 for the regression.
Comment #19 by bugzilla — 2013-10-20T09:38:57Z
This pull: https://github.com/D-Programming-Language/dmd/pull/2673 undoes it with explanation. Reclassified as "wontfix".
Comment #20 by bearophile_hugs — 2013-10-20T10:49:58Z
(In reply to comment #19) > This pull: > > https://github.com/D-Programming-Language/dmd/pull/2673 > > undoes it with explanation. Reclassified as "wontfix". What? After all the energy time and work spent on fixing this problem!? If you take a look at the patch by Kenji you see: - else if (global.params.warnings && !global.gag && op == TOKassign && + else if (0 && global.params.warnings && !global.gag && op == TOKassign && That means the code is commented out, but it's meant to be activated back in dmd 2.065 to try to fix this issue better. I will probably re-open this issue later.
Comment #21 by bearophile_hugs — 2013-10-20T12:29:43Z
Reopened, this will wait for dmd 2.065, to see if Kenji or others are able to find a solution. If no solution will be found, we'll close this again.
Comment #22 by dfj1esp02 — 2014-09-11T06:40:48Z
That's the reason for an unambiguous syntax a[*]=b[*];
Comment #23 by peter.alexander.au — 2018-08-24T12:14:31Z
I just lost a bunch of time because of this issue (T[n] = e). I understand this is a breaking change. Is there perhaps a deprecation path to solving this?
Comment #24 by nick — 2024-07-19T17:16:41Z
I think `[]` should be required at least for assigning a single element to a static array, like it is for a dynamic array. Another issue is with an AA of static array key type: https://forum.dlang.org/post/[email protected] // meant to create key "test" and copy slice as value lookup["test"] = dynArray[0 .. $]; // instead causes a missing key error as it becomes lookup["test"][0 .. $] = dynArray[0 .. $];
Comment #25 by nick — 2024-07-19T17:31:21Z
> Another issue is with an AA of static array key type: Sorry, static array *value* type: string[3][string] lookup; string[] dynArray = ["d", "e", "f"];
Comment #26 by robert.schadek — 2024-12-13T17:58:18Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/18411 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB