www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - The lifetime of reduce's internal seed

reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
I opened the following bug before reading reduce's documentation carefully:

   https://issues.dlang.org/show_bug.cgi?id=12610

import std.stdio;
import std.algorithm;

void main()
{
     int[] arr = [ 0 ];

     int[1] seed;
     int[] result = reduce!((sum, _) => sum[])(seed, arr);

     writefln("%s", result);
}

The output is garbage like [373728176].

Note that the original seed is a value type, which is later sliced by 
the lambda.

Now I think that it can be considered a user error. Do you agree whit 
that? If so, this is something else we should be careful about similar 
to the internal buffer of byLine.

Ali
Apr 22 2014
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 22 April 2014 at 17:31:22 UTC, Ali Çehreli wrote:

 I opened the following bug before reading reduce's 
 documentation carefully:

   https://issues.dlang.org/show_bug.cgi?id=12610

 import std.stdio;
 import std.algorithm;

 void main()
 {
     int[] arr = [ 0 ];

     int[1] seed;
     int[] result = reduce!((sum, _) => sum[])(seed, arr);

     writefln("%s", result);
 }

 The output is garbage like [373728176].

 Note that the original seed is a value type, which is later 
 sliced by the lambda.

 Now I think that it can be considered a user error. Do you 
 agree whit that? If so, this is something else we should be 
 careful about similar to the internal buffer of byLine.

 Ali


I see one user bug, and one language bug, but no library issues.

The user bug is the lambda, that returns a reference to a local 
("sum[]"). That said, I do believe the compiler should be able to 
catch think.

The "int[] reduce...", however, I think is a outright language 
issue. Implicilty calling opSlice on a static array is one thing, 
but doing it on an rvalue is an outright aberration. The code 
should be rejected no questions asked.
Apr 22 2014
parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 04/22/2014 11:03 AM, monarch_dodra wrote:


 On Tuesday, 22 April 2014 at 17:31:22 UTC, Ali Çehreli wrote:

 I opened the following bug before reading reduce's documentation
 carefully:

   https://issues.dlang.org/show_bug.cgi?id=12610

 import std.stdio;
 import std.algorithm;

 void main()
 {
     int[] arr = [ 0 ];

     int[1] seed;
     int[] result = reduce!((sum, _) => sum[])(seed, arr);

     writefln("%s", result);
 }

 The output is garbage like [373728176].

 Note that the original seed is a value type, which is later sliced by
 the lambda.

 Now I think that it can be considered a user error. Do you agree whit
 that? If so, this is something else we should be careful about similar
 to the internal buffer of byLine.

 Ali


 I see one user bug,


Let this thread be a lesson to me (and other mortals) then.


 and one language bug, but no library issues.

 The user bug is the lambda, that returns a reference to a local
 ("sum[]"). That said, I do believe the compiler should be able to catch
 think.

 The "int[] reduce...", however, I think is a outright language issue.
 Implicilty calling opSlice on a static array is one thing, but doing it
 on an rvalue is an outright aberration. The code should be rejected no
 questions asked.


I don't think there is slicing an rvalue though. (?) reduce() is taking 
a copy of the seed and then returning a slice to it because the user 
slices it in their lambda. It effectively does the following, which 
unfortunately compiles:

int[] foo()
{
     int[1] sum;
     return sum[];    // <-- no warning
}

void main()
{
     int[] result = foo();
}

The code would be safe if the user gave a slice to reduce(). This brings 
up another concern though: Template authors must watch out for cases 
like the above. There would be cases where we shouldn't simply take a 
copy of a T object and return it.

Ali
Apr 22 2014
next sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Tue, 22 Apr 2014 14:17:57 -0400, Ali =C3=87ehreli <acehreli yahoo.com=

 wrote:



 I don't think there is slicing an rvalue though. (?) reduce() is takin=


g  =


 a copy of the seed and then returning a slice to it because the user  =



 slices it in their lambda. It effectively does the following, which  =



 unfortunately compiles:

 int[] foo()
 {
      int[1] sum;
      return sum[];    // <-- no warning
 }


It's not slicing an rvalue, but the above is trivially no different than=
:

int[] foo()
{
    int[1] sum;
    return sum;
}

which does NOT compile.

The issue is that D's escape analysis is very very simplistic. It would =
be  =

nice if it were better.

Ironically, because of return type inference in lambdas, you circumvente=
d  =

the check!

-Steve
Apr 22 2014
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 22 April 2014 at 18:34:47 UTC, Steven Schveighoffer 
wrote:

 On Tue, 22 Apr 2014 14:17:57 -0400, Ali Çehreli 
 <acehreli yahoo.com> wrote:


 I don't think there is slicing an rvalue though. (?) reduce() 
 is taking a copy of the seed and then returning a slice to it 
 because the user slices it in their lambda. It effectively 
 does the following, which unfortunately compiles:

 int[] foo()
 {
     int[1] sum;
     return sum[];    // <-- no warning
 }


 It's not slicing an rvalue, but the above is trivially no 
 different than:


In this case no, but;
//----
int[1] foo();
int[] a = foo();
//----
*is* slicing an rvalue, and it *does* compile. I don't think 
there needs to be escape analysis to catch this.
Apr 22 2014
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Tue, 22 Apr 2014 14:47:19 -0400, monarch_dodra <monarchdodra gmail.com>  
wrote:


 In this case no, but;
 //----
 int[1] foo();
 int[] a = foo();
 //----
 *is* slicing an rvalue, and it *does* compile. I don't think there needs  
 to be escape analysis to catch this.


Oh yeah, that's bad.

-Steve
Apr 22 2014
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 22 April 2014 at 18:49:41 UTC, Steven Schveighoffer 
wrote:

 On Tue, 22 Apr 2014 14:47:19 -0400, monarch_dodra 
 <monarchdodra gmail.com> wrote:


 In this case no, but;
 //----
 int[1] foo();
 int[] a = foo();
 //----
 *is* slicing an rvalue, and it *does* compile. I don't think 
 there needs to be escape analysis to catch this.


 Oh yeah, that's bad.

 -Steve


It's filed:
https://issues.dlang.org/show_bug.cgi?id=12625

I hope it gets fixed. I really see no justification for it.
Apr 23 2014
prev sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 22 April 2014 at 18:17:58 UTC, Ali Çehreli wrote:

 On 04/22/2014 11:03 AM, monarch_dodra wrote:

 The "int[] reduce...", however, I think is a outright


 language issue.

 Implicilty calling opSlice on a static array is one thing,


 but doing it

 on an rvalue is an outright aberration. The code should be


 rejected no

 questions asked.


 I don't think there is slicing an rvalue though. (?) reduce() 
 is taking a copy of the seed and then returning a slice to it 
 because the user slices it in their lambda. It effectively does 
 the following, which unfortunately compiles:

 int[] foo()
 {
     int[1] sum;
     return sum[];    // <-- no warning
 }


Reduce returns "the seed". It's actually doing something more 
like this:

int[1] foo()
{
     int[1] sum
     sum = sum[]; //The lambda operates, and the
                  //result is assigned back to the seed.
     return sum; //Returns the seed.
}

So when writing:


 void main()
 {
     int[] result = foo();
 }


There's a *second* level of bug. (assigning an rvalue "int[1]" to 
int[])

*This* works fine though:
     int[1] seed;
     int[1] result = reduce!((ref sum, _) => sum)(seed, arr);

BTW, I'm re-implemented reduce recently (not yet pulled), but I 
was *very* thorough about documenting what it does:
https://github.com/D-Programming-Language/phobos/pull/2060

Could you take a look at it (the documentation I mean), and tell 
me if everything is what you would have expected?
Apr 22 2014
parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 04/22/2014 11:45 AM, monarch_dodra wrote:


 Reduce returns "the seed". It's actually doing something more like this:

 int[1] foo()
 {
      int[1] sum
      sum = sum[]; //The lambda operates, and the
                   //result is assigned back to the seed.
      return sum; //Returns the seed.
 }


My original lambda that returned a slice was correct then. The seed 
would eventually be copied out. Had the compiler not allow slicing the 
rvalue then I would be in good shape.


 BTW, I'm re-implemented reduce recently (not yet pulled), but I was
 *very* thorough about documenting what it does:
 https://github.com/D-Programming-Language/phobos/pull/2060

 Could you take a look at it (the documentation I mean), and tell me if
 everything is what you would have expected?


I think it looks great! :)

Two comments/questions which I did not make on github:

1) Some of the documentation comments that are inside a scope are not 
formatted as such. For example, this comment does not start with /++ :

 
https://github.com/monarchdodra/phobos/blob/reduceReimpl/std/algorithm.d#L753

I wonder whether they are still included in the produced documentation.

2) I think even single-line code blocks should have curly brackets but 
Phobos code does not follow that guideline. :)

Ali
Apr 25 2014
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Saturday, 26 April 2014 at 06:24:26 UTC, Ali Çehreli wrote:

 On 04/22/2014 11:45 AM, monarch_dodra wrote:


 Reduce returns "the seed". It's actually doing something more


 like this:

 int[1] foo()
 {
      int[1] sum
      sum = sum[]; //The lambda operates, and the
                   //result is assigned back to the seed.
      return sum; //Returns the seed.
 }


 My original lambda that returned a slice was correct then. The 
 seed would eventually be copied out. Had the compiler not allow 
 slicing the rvalue then I would be in good shape.


Well... your lambda *was* returning a slice to its local copy of 
sum. So I thin kit is still wrong. "(ref sum, _) => sum[]" would 
have been correct though.


 BTW, I'm re-implemented reduce recently (not yet pulled), but


 I was

 *very* thorough about documenting what it does:
 https://github.com/D-Programming-Language/phobos/pull/2060

 Could you take a look at it (the documentation I mean), and


 tell me if

 everything is what you would have expected?


 I think it looks great! :)

 Two comments/questions which I did not make on github:

 1) Some of the documentation comments that are inside a scope 
 are not formatted as such. For example, this comment does not 
 start with /++ :


 https://github.com/monarchdodra/phobos/blob/reduceReimpl/std/algorithm.d#L753

 I wonder whether they are still included in the produced 
 documentation.


Nope, that was a mistake on my part. Good catch.


 2) I think even single-line code blocks should have curly 
 brackets but Phobos code does not follow that guideline. :)

 Ali


It depends I say. I usually do that, but for certain functions, 
such as reduce, it would *literally* double the amount of lines 
required to write it. I that point, the function becomes long 
enough for it to be a readability problem.
Apr 26 2014