www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Assigning a static array

reply "Brad Anderson" <eco gnuk.net> writes:
Is this supposed to be allowed:

ubyte[] a;
ubyte[16] b;
a = b;
assert(a.ptr == b.ptr);

Because if so that makes it terribly easy to do a bug like this 
(as I just saw in IRC):

struct A
{
     ubyte[] a;
     this(ubyte c)
     {
         ubyte[16] b;
         b[] = c;
         this.a = b;  // a now points at an immediately invalid 
static array
     }
}
Apr 18 2013
next sibling parent "Brad Anderson" <eco gnuk.net> writes:
For reference, here was what user soos on IRC was doing that 


import std.digest.md;
import std.stdio;

struct Hash {
   ubyte[] hash1;
   ubyte[] hash2;

   this (string str) {
     auto md5 = new MD5Digest();
     this.hash1 = md5.digest(str);
     this.hash2 = digest!MD5(str);
   }
};

void main() {
   auto h = Hash("hello world!");
   writeln(toHexString(h.hash1));
   writeln(toHexString(h.hash2));
}


It's not obvious at all what the problem is.
Apr 18 2013
prev sibling next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, April 18, 2013 23:06:32 Brad Anderson wrote:

 Is this supposed to be allowed:
 
 ubyte[] a;
 ubyte[16] b;
 a = b;
 assert(a.ptr == b.ptr);
 
 Because if so that makes it terribly easy to do a bug like this
 (as I just saw in IRC):
 
 struct A
 {
 ubyte[] a;
 this(ubyte c)
 {
 ubyte[16] b;
 b[] = c;
 this.a = b; // a now points at an immediately invalid
 static array
 }
 }


Yes, that's legal, though it should arguably be  system, since it's doing 
essentially the same thing as

int i;
int* p = &i;

which _is_  system. The compiler doesn't currently consider slicing a static 
array in general to be  system though, much as it should ( 
http://d.puremagic.com/issues/show_bug.cgi?id=8838 ). I could see an argument 
that it should have to be

a = b[];

so that the slicing is explicit instead of just

a = b;

where it's implicit, but AFAIK, that's not currently required. You have to be 
very careful when slicing static arrays. If it were up to me, exlicit slicing 
would _always_ be required when slicing a static array, even when calling 
functions which take a dynamic array, but that's not how it works 
unfortunately.

- Jonathan M Davis
Apr 18 2013
parent "bearophile" <bearophileHUGS lycos.com> writes:
Jonathan M Davis:


 I could see an argument that it should have to be

 a = b[];

 so that the slicing is explicit instead of just

 a = b;

 where it's implicit, but AFAIK, that's not currently required.


It's currently a warning, and it will be required.

--------------------------

Ali Çehreli:


 There is a similar problem with the automatically generated 
 array arguments.

 The following constructor takes any number of ints that come in 
 array form:

 import std.stdio;

 struct S
 {
     int[] a;

     this(int[] args...)
     {
         a = args;
     }

     void foo()
     {
         writeln(a);
     }
 }

 void main()
 {
     S[] a;

     foreach (i; 0 .. 2) {
         a ~= S(i, i, i);  // <-- WARNING temporary array


This is a known problem, I have put it in Bugzilla since lot of 
time and it seems there are various persons that agree with me 
and you on it. So hopefully this source of bugs will be removed, 
allocating that data on the heap.

Bye,
bearophile
Apr 18 2013
prev sibling next sibling parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 04/18/2013 02:06 PM, Brad Anderson wrote:

 Is this supposed to be allowed:

 ubyte[] a;
 ubyte[16] b;
 a = b;
 assert(a.ptr == b.ptr);

 Because if so that makes it terribly easy to do a bug like this (as I
 just saw in IRC):

 struct A
 {
      ubyte[] a;
      this(ubyte c)
      {
          ubyte[16] b;
          b[] = c;
          this.a = b;  // a now points at an immediately invalid static
 array
      }
 }


There is a similar problem with the automatically generated array arguments.

The following constructor takes any number of ints that come in array form:

import std.stdio;

struct S
{
     int[] a;

     this(int[] args...)
     {
         a = args;
     }

     void foo()
     {
         writeln(a);
     }
}

void main()
{
     S[] a;

     foreach (i; 0 .. 2) {
         a ~= S(i, i, i);  // <-- WARNING temporary array
     }

     foreach (e; a) {
         e.foo();
     }
}

The program prints the following because the temporary arrays that are 
generated when calling the constructors are long gone:

[1, 1, 1]
[1, 1, 1]

The programmer *may have* ;) expected the following output:

[1, 1, 1]
[2, 2, 2]

Ali
Apr 18 2013
next sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
 There is a similar problem with the automatically generated array  
 arguments.

 The following constructor takes any number of ints that come in array  
 form:

 import std.stdio;

 struct S
 {
      int[] a;

      this(int[] args...)
      {
          a = args;
      }

      void foo()
      {
          writeln(a);
      }
 }

 void main()
 {
      S[] a;

      foreach (i; 0 .. 2) {
          a ~= S(i, i, i);  // <-- WARNING temporary array
      }

      foreach (e; a) {
          e.foo();
      }
 }

 The program prints the following because the temporary arrays that are  
 generated when calling the constructors are long gone:

 [1, 1, 1]
 [1, 1, 1]

 The programmer *may have* ;) expected the following output:

 [1, 1, 1]
 [2, 2, 2]


There is no guarantee that the incoming array from a variadic function is  
heap-based.

But an interesting way to deal with it is that you can overload with an  
explicit slice parameter, and the variadic version will ONLY bind to a  
variadic call.

For example, in dcollections' ArrayList I have two constructors:

    /**
      * Create an array list based on a number of elements.
      */
     this(V[] elems...)
     {
         _array = elems.dup;
     }

     /**
      * Use an array as the backing storage.  This does not duplicate the
      * array.  Use new ArrayList(storage.dup) to make a distinct copy.   
Beware
      * of using a stack-based array when using this constructor!
      */
     this(V[] storage)
     {
         _array = storage;
     }

The first version binds only to cases where the parameter is not  
*explicitly* a slice, so I am guaranteed that the array is allocated on  
the stack!  The second binds to slices, and I assume from my comment,  
fixed-sized arrays (been a while since I looked at that code).

And the correct expectation for your code should be:

[0, 0, 0]
[1, 1, 1]

:)

-Steve
Apr 18 2013
next sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
Steven Schveighoffer:


 There is no guarantee that the incoming array from a variadic 
 function is heap-based.

 But an interesting way to deal with it is that you can overload 
 with an explicit slice parameter, and the variadic version will 
 ONLY bind to a variadic call.

 For example, in dcollections' ArrayList I have two constructors:

    /**
      * Create an array list based on a number of elements.
      */
     this(V[] elems...)
     {
         _array = elems.dup;
     }

     /**
      * Use an array as the backing storage.  This does not 
 duplicate the
      * array.  Use new ArrayList(storage.dup) to make a 
 distinct copy.  Beware
      * of using a stack-based array when using this constructor!
      */
     this(V[] storage)
     {
         _array = storage;
     }


To avoid those bugs I have suggested the simpler possible thing: 
(V[] elems...) to dup the data on the heap every time. In theory 
if you write "(scope V[] elems...)" it will be free to not dup 
the data, avoiding the heap allocation and the associated little 
performance loss. In practice as you know "scope" is not yet 
implemented. D2 language is not close to being fully implemented.

Bye,
bearophile
Apr 18 2013
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 18 Apr 2013 18:08:48 -0400, bearophile <bearophileHUGS lycos.com>
wrote:


 To avoid those bugs I have suggested the simpler possible thing: (V[]  
 elems...) to dup the data on the heap every time. In theory if you write  
 "(scope V[] elems...)" it will be free to not dup the data, avoiding the  
 heap allocation and the associated little performance loss. In practice  
 as you know "scope" is not yet implemented. D2 language is not close to  
 being fully implemented.


I am OK with this as long as it is done AFTER scope works (and I would
update my calls appropriately).

But in my specific case, I still want the second overload, because the
idea is ArrayList uses that slice as its actual storage.

-Steve
Apr 19 2013
prev sibling parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 04/18/2013 02:54 PM, Steven Schveighoffer wrote:


 The program prints the following because the temporary arrays that are
 generated when calling the constructors are long gone:

 [1, 1, 1]
 [1, 1, 1]

 The programmer *may have* ;) expected the following output:

 [1, 1, 1]
 [2, 2, 2]


 There is no guarantee that the incoming array from a variadic function
 is heap-based.

 But an interesting way to deal with it is that you can overload with an
 explicit slice parameter, and the variadic version will ONLY bind to a
 variadic call.


Interesting.

Personally, I would not even bother with the second one and expect the 
caller to simply put square brackets around the arguments:

import std.stdio;

struct S
{
     int[] a;

     this(int[] args)  // <-- now requires a slice
     {
         a = args;
     }

     void foo()
     {
         writeln(a);
     }
}

void main()
{
     S[] a;

     foreach (i; 0 .. 2) {
         a ~= S([i, i, i]); // <-- minor inconvenience
     }

     foreach (e; a) {
         e.foo();
     }
}

It is now safe, right?


 And the correct expectation for your code should be:

 [0, 0, 0]
 [1, 1, 1]

 :)

 -Steve


Wow! I made an off-by-6 error there. :)

Ali
Apr 18 2013
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 18 Apr 2013 18:13:14 -0400, Ali =C3=87ehreli <acehreli yahoo.com=

 wrote:




 Interesting.

 Personally, I would not even bother with the second one and expect the=


  =


 caller to simply put square brackets around the arguments:


[snip]


 It is now safe, right?


Yes, for this case it is OK.  But I want a consistent interface in
dcollections.  Other containers do not use arrays as storage, so the
bracketed version for something like a TreeSet would be a complete waste=

(and in fact, only the variadic version exists there).  The special case=

for ArrayList is the slice version, where it actually takes on that slic=
e
as its internal storage.  ArrayList is meant to bridge the gap between D=

slices and dcollections.  It's range type is a D slice as well.

-Steve
Apr 19 2013
prev sibling parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Thu, Apr 18, 2013 at 02:43:54PM -0700, Ali Çehreli wrote:

 On 04/18/2013 02:06 PM, Brad Anderson wrote:

Is this supposed to be allowed:

ubyte[] a;
ubyte[16] b;
a = b;
assert(a.ptr == b.ptr);

Because if so that makes it terribly easy to do a bug like this (as I
just saw in IRC):

struct A
{
     ubyte[] a;
     this(ubyte c)
     {
         ubyte[16] b;
         b[] = c;
         this.a = b;  // a now points at an immediately invalid static
array
     }
}


 
 There is a similar problem with the automatically generated array arguments.
 
 The following constructor takes any number of ints that come in array form:
 
 import std.stdio;
 
 struct S
 {
     int[] a;
 
     this(int[] args...)
     {
         a = args;
     }
 
     void foo()
     {
         writeln(a);
     }
 }


[...]

Yeah I got bitten by this before. Took me several days to find the
problem, 'cos it was nested deep inside a complex data structure, and at
a glance it doesn't *look* wrong.

I'm all for making this  system at the very least, if not outright
compile error. Storing a persistent reference to a stack-allocated
object is outright wrong... in the case of variadic array args, if the
compiler can't prove that args will *always* be a dynamic array, then
any attempt to save a reference to it should be rejected outright IMO.


T

-- 
It is impossible to make anything foolproof because fools are so ingenious. --
Sammy
Apr 18 2013
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 18 Apr 2013 19:15:06 -0400, H. S. Teoh <hsteoh quickfur.ath.cx>  
wrote:


 I'm all for making this  system at the very least, if not outright
 compile error. Storing a persistent reference to a stack-allocated
 object is outright wrong... in the case of variadic array args, if the
 compiler can't prove that args will *always* be a dynamic array, then
 any attempt to save a reference to it should be rejected outright IMO.


I like bearophile's solution, we increasingly need scope to work for D to  
be easy to avoid mistakes.

-Steve
Apr 19 2013
next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Steven Schveighoffer:


 we increasingly need scope to work for D to be easy to avoid 
 mistakes.


But I don't see any plan/implementation timeframe for "scope" :-( 
I don't know why. I'd like to know Hara opinion on this.

Bye,
bearophile
Apr 19 2013
prev sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Steven Schveighoffer:


 we increasingly need scope to work for D to be easy to avoid 
 mistakes.


I have added a note about scope:
http://d.puremagic.com/issues/show_bug.cgi?id=5212

Bye,
bearophile
Apr 19 2013
prev sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
Brad Anderson:


 Is this supposed to be allowed:

 ubyte[] a;
 ubyte[16] b;
 a = b;
 assert(a.ptr == b.ptr);


Yes, this is supposed to be allowed with the current design of D. 
But with the latest dmd 2.063alpha that code doesn't work, see 
below.

The Rust language removes this source of bugs because it manages 
accurately the memory zones. D will probably improve its error 
detection capabilities for such simple cases, but I think it will 
not solve this problem in general, unless it improves its type 
system, similarly to Rust.



 import std.digest.md;
 import std.stdio;

 struct Hash {
   ubyte[] hash1;
   ubyte[] hash2;

   this (string str) {
     auto md5 = new MD5Digest();
     this.hash1 = md5.digest(str);
     this.hash2 = digest!MD5(str);
   }
 };

 void main() {
   auto h = Hash("hello world!");
   writeln(toHexString(h.hash1));
   writeln(toHexString(h.hash2));
 }


 It's not obvious at all what the problem is.


Now that code gives a warning:

temp.d(11): Warning: explicit slice assignment this.hash2 = 
(digest(str))[] is better than this.hash2 = digest(str)

(This is the result of a small battle I have started years ago 
that is now half work, thanks to the work of Hara implementing my 
enhancement request. I am glad to see our time was not wasted.)

To remove that warning you have to write:

this.hash1 = md5.digest(str);
this.hash2 = digest!MD5(str)[];

Now it's visible that for hash2 you are slicing something that's 
probably a fixed-sized array. This gives you a significant help 
in understanding what's going on.

Bye,
bearophile
Apr 18 2013
parent "Brad Anderson" <eco gnuk.net> writes:
On Thursday, 18 April 2013 at 21:45:56 UTC, bearophile wrote:

 Yes, this is supposed to be allowed with the current design of 
 D. But with the latest dmd 2.063alpha that code doesn't work, 
 see below.

 [snip]

 Now that code gives a warning:

 temp.d(11): Warning: explicit slice assignment this.hash2 = 
 (digest(str))[] is better than this.hash2 = digest(str)

 (This is the result of a small battle I have started years ago 
 that is now half work, thanks to the work of Hara implementing 
 my enhancement request. I am glad to see our time was not 
 wasted.)

 To remove that warning you have to write:

 this.hash1 = md5.digest(str);
 this.hash2 = digest!MD5(str)[];

 Now it's visible that for hash2 you are slicing something 
 that's probably a fixed-sized array. This gives you a 
 significant help in understanding what's going on.


That's good to hear, Bearophile. Thanks Kenji.
Apr 18 2013