Here's an example:
import std.stdio;
void main() {
invariant(int)[] x = [1,4,2,3];
x.sort;
writefln(x); // [1,2,3,4]
x.reverse;
writefln(x); // [4,3,2,1]
x.length = x.length - 1;
x.length = x.length + 1;
writefln(x); // [4,3,2,0]
}
The properties .sort and .reverse should be simply disallowed for invariant arrays. I'm not sure about the .length property, but I think some consideration must be given...
Comment #1 by smjg — 2009-01-10T11:43:09Z
This is closely related to bug 2544.
Comment #2 by smjg — 2009-02-18T15:51:27Z
Did I mean 2544? In any case, bug 2093 is one that the .length bit especially certainly is related to.
Comment #3 by dfj1esp02 — 2009-02-19T02:55:20Z
It reallocated array on increasing length. Seems valid.
Comment #4 by 2korden — 2009-02-19T03:27:04Z
(In reply to comment #3)
> It reallocated array on increasing length. Seems valid.
>
Yes, the length part is fine. Here is slightly modified test case:
import std.stdio;
import std.algorithm;
void main()
{
immutable(int)[] x = [1, 5, 3, 4];
auto y = x;
writefln(y);
x.sort;
writefln(y); // immutable variable is modified!
x.reverse;
writefln(y);
}
Problem is that sort and reverse work in-place, thus modifying immutable variable. It should either not compile /or/ allocate a copy. I don't like the hidden allocations, so this should be just disallowed, imo.
Comment #5 by smjg — 2009-02-19T03:41:16Z
(In reply to comment #4)
> (In reply to comment #3)
>> It reallocated array on increasing length. Seems valid.
What did - the testcase here with checking added, the testcase at bug 2093 or your own? With which DMD version?
> Problem is that sort and reverse work in-place, thus modifying immutable
> variable. It should either not compile /or/ allocate a copy. I don't like the
> hidden allocations, so this should be just disallowed, imo.
Indeed, to either modify in-place or reallocate depending on constancy status would be a confusing inconsistency. They should be distinct operations. Indeed, you can already do a reallocating sort or reverse by doing
int[] sorted = x.dup.sort;
invariant(int)[] = assumeUnique(x.dup.reverse);
so it would be a question of whether it's worth creating syntactic sugar for this.
Comment #6 by dfj1esp02 — 2009-02-19T04:04:49Z
> What did? With which DMD version?
The first testcase did. DMD version is specified in the bugreport. My version is 2.023.
Comment #7 by smjg — 2009-02-19T13:59:33Z
Oh yes. Increasing .length always reallocates under 2.025, whether const, invariant or not. The problem is that concatenation doesn't.
Comment #8 by 2korden — 2009-02-19T18:06:27Z
(In reply to comment #7)
> Oh yes. Increasing .length always reallocates under 2.025, whether const,
> invariant or not. The problem is that concatenation doesn't.
>
Do you say that the following:
char[] s;
for (int i = 0; i < 10_000_000; ++i) {
s ~= '0';
// or:
// s.length = s.length + 1;
// s[$-1] = '0';
}
will reallocate on *each* iteration?
While this solves one issue, it opens another one - big performance drop (create new report?)
Comment #9 by schveiguy — 2010-03-16T20:06:45Z
The length property problems of this bug are fixed via the patch in bug 3637. This was incorporated in dmd 2.041.
The original test case for the length was invalid.
the code still prints 4 3 2 0, because it is OK to append to an invariant(int)[].
A valid test that would have failed prior to 2.041:
import std.stdio;
void main()
{
invariant(int)[] x = [1,2,3,4];
auto y = x;
x.length = x.length - 1;
x.length = x.length + 1;
writeln(y); // prints 1 2 3 4 (prior to 2.041 would print 1 2 3 0)
writeln(x); // prints 1 2 3 0
}
The sort and reverse property bugs remain valid bugs.
Comment #10 by clugdbug — 2010-05-06T13:33:54Z
Dunno why I bothered with this one, since we should just ditch .sort and .reverse.
But anyway...
PATCH: expression.c DotIdExp::semantic(), around line 6113.
else if (t1b->ty == Tarray ||
t1b->ty == Tsarray ||
t1b->ty == Taarray)
{ /* If ident is not a valid property, rewrite:
* e1.ident
* as:
* .ident(e1)
*/
TypeArray *tarr = (TypeArray *)t1b;
if (ident == Id::sort && !tarr->next->isMutable()) {
error(".sort only applies to mutable arrays");
return new ErrorExp();
}
if (ident == Id::reverse && !tarr->next->isMutable()) {
error(".reverse only applies to mutable arrays");
return new ErrorExp();
}
Comment #11 by schveiguy — 2010-05-06T13:38:49Z
Don, while you are looking at this, have a look at related bug 3550
Comment #12 by bearophile_hugs — 2010-05-06T13:59:43Z
I think array reverse can be kept.
Comment #13 by clugdbug — 2010-05-10T01:44:42Z
There's a bit of code in druntime, in rt/adi.d which is affected by this.
extern (C) long _adSortChar(char[] a)
{
if (a.length > 1)
{
- dstring da = toUTF32(a);
+ dchar [] da = cast(dchar[])(toUTF32(a));
da.sort;
and likewise for _adSortWChar.
But it'd probably be better to modify rt/util/utf/toUTF32().
Comment #14 by bearophile_hugs — 2011-04-06T16:56:23Z
The same happens with enum arrays:
enum int[] data = [3, 1, 2];
void main() {
data.sort;
}
Comment #15 by smjg — 2011-04-09T08:13:36Z
(In reply to comment #14)
> The same happens with enum arrays:
>
> enum int[] data = [3, 1, 2];
> void main() {
> data.sort;
> }
Array enums have enough problems of their own. See issue 2331, issue 2414 and probably others.