Running a piece of code that can be reduced to:

---
import std.stdio;

void main()
{
	import std.range;
	foreach(item; iota(0, 10).transform(2))
		writeln(item);
}

auto transform(T)(T list, real x)
{
	auto t = /* new */ Transformer(x);   // line 12
	return t.applyTo(list);
}

struct Transformer
{
	real delegate(real) fun;

	this(real x)
	{
		fun = (real r) => r * x;
	}

	auto applyTo(T)(T list)
	{
		import std.algorithm;
		return list.map!(x => fun(x));
	}
}
---

the program segfaults. I guess it's because fun is destroyed when 't' goes out of scope in 'transform'. I would have thought that the MapResult struct returned by 'applyTo' still holds a valid copy of fun, but I'm wrong... Is there a way to do it?

Of course, uncommenting 'new' on line 12 resolves the problem.

Thanks,
Nicolas

On Sun, 26 Jan 2014 18:41:00 -0500, Nicolas Sicard <dransic@gmail.com> wrote:

> Running a piece of code that can be reduced to:
>
> ---
> import std.stdio;
>
> void main()
> {
> 	import std.range;
> 	foreach(item; iota(0, 10).transform(2))
> 		writeln(item);
> }
>
> auto transform(T)(T list, real x)
> {
> 	auto t = /* new */ Transformer(x);   // line 12
> 	return t.applyTo(list);
> }
>
> struct Transformer
> {
> 	real delegate(real) fun;
>
> 	this(real x)
> 	{
> 		fun = (real r) => r * x;
> 	}
>
> 	auto applyTo(T)(T list)
> 	{
> 		import std.algorithm;
> 		return list.map!(x => fun(x));
> 	}
> }
> ---
>
> the program segfaults. I guess it's because fun is destroyed when 't' goes out of scope in 'transform'. I would have thought that the MapResult struct returned by 'applyTo' still holds a valid copy of fun, but I'm wrong... Is there a way to do it?

No. Just don't do that. The runtime is permitted to move structs bit-for-bit, so you are not allowed to store a pointer that references 'this'. Unless you prevent copying via @disable this(this), your struct could be moved if someone, for instance, passed it as a parameter on the stack, or returned it.

A delegate using 'this' as the context pointer is the same thing.

The only way to solve this is to put the struct on the heap. But why even use a struct? You could just use a closure (which automatically goes on the heap if needed):

auto Transformer(real x)
{
  return (real r) => r * x;
}

auto applyTo(X, T)(X fun, T list)
{
   import std.algorithm;
   return list.map!(x => fun(x));
}

auto transform(T)(T list, real x)
{
   auto t = Transformer(x);
   return t.applyTo(list);
}

-Steve

On Monday, 27 January 2014 at 02:27:08 UTC, Steven Schveighoffer wrote:
> On Sun, 26 Jan 2014 18:41:00 -0500, Nicolas Sicard <dransic@gmail.com> wrote:
>
>> Running a piece of code that can be reduced to:
>>
>> ---
>> import std.stdio;
>>
>> void main()
>> {
>> 	import std.range;
>> 	foreach(item; iota(0, 10).transform(2))
>> 		writeln(item);
>> }
>>
>> auto transform(T)(T list, real x)
>> {
>> 	auto t = /* new */ Transformer(x);   // line 12
>> 	return t.applyTo(list);
>> }
>>
>> struct Transformer
>> {
>> 	real delegate(real) fun;
>>
>> 	this(real x)
>> 	{
>> 		fun = (real r) => r * x;
>> 	}
>>
>> 	auto applyTo(T)(T list)
>> 	{
>> 		import std.algorithm;
>> 		return list.map!(x => fun(x));
>> 	}
>> }
>> ---
>>
>> the program segfaults. I guess it's because fun is destroyed when 't' goes out of scope in 'transform'. I would have thought that the MapResult struct returned by 'applyTo' still holds a valid copy of fun, but I'm wrong... Is there a way to do it?
>
> No. Just don't do that. The runtime is permitted to move structs bit-for-bit, so you are not allowed to store a pointer that references 'this'. Unless you prevent copying via @disable this(this), your struct could be moved if someone, for instance, passed it as a parameter on the stack, or returned it.
>
> A delegate using 'this' as the context pointer is the same thing.
>
> The only way to solve this is to put the struct on the heap. But why even use a struct? You could just use a closure (which automatically goes on the heap if needed):
>
> auto Transformer(real x)
> {
>   return (real r) => r * x;
> }
>
> auto applyTo(X, T)(X fun, T list)
> {
>    import std.algorithm;
>    return list.map!(x => fun(x));
> }
>
> auto transform(T)(T list, real x)
> {
>    auto t = Transformer(x);
>    return t.applyTo(list);
> }
>
> -Steve

Actually I used a struct because the code is more complex, and it builds an array of delegates, which are returned from global functions, like:
---
struct Transformer
{
	real delegate(real)[] funs;

	addFun(real x)
	{
		fun ~= makeFun(x);
	}

	// etc.
}

real delegate(real) makeFun(real x)
{
	return (real r) => r * x;
}
---

This means my design was bad in the first place.
Thanks for the explanation.

Nicolas

On Mon, 27 Jan 2014 03:17:51 -0500, Nicolas Sicard <dransic@gmail.com> wrote:

> Actually I used a struct because the code is more complex, and it builds an array of delegates, which are returned from global functions, like:
> ---
> struct Transformer
> {
> 	real delegate(real)[] funs;
>
> 	addFun(real x)
> 	{
> 		fun ~= makeFun(x);
> 	}
>
> 	// etc.
> }
>
> real delegate(real) makeFun(real x)
> {
> 	return (real r) => r * x;
> }
> ---
>
> This means my design was bad in the first place.
> Thanks for the explanation.

Actually, the delegate there is fine! The makeFun function becomes a closure, and will be allocated on the heap.

Where you are running into trouble is simply that the struct goes out of scope, and the array is therefore invalid.

In fact, I think you were already doing that before (creating a closure).

Here is a possible solution to your original example:

	auto applyTo(T)(T list)
	{
		import std.algorithm;
                auto funcopy = fun;
		return list.map!(x => funcopy(x));
	}

What's happening here is that funcopy is a stack local variable. However, since you're creating a delegate that uses local variables, the compiler creates a closure. In essence, it's like putting a new struct on the heap with the single member funcopy, and using that as the context pointer. Note that the original code also creates a closure, but 'fun' is a member of the hidden 'this' reference. Because the 'this' reference refers to destructed data, fun is garbage, hence the segfault.

I actually was wrong about my original diagnosis. The delegate stored in your original code does NOT store a delegate with a context pointer that points to 'this', it's pointing to a closure. Because 'x' doesn't exist inside the struct, only inside the function. But my statements were still good advice, don't store pointers to yourself inside a struct :)

-Steve

On Monday, 27 January 2014 at 14:47:21 UTC, Steven Schveighoffer wrote:
> On Mon, 27 Jan 2014 03:17:51 -0500, Nicolas Sicard <dransic@gmail.com> wrote:
>
>> Actually I used a struct because the code is more complex, and it builds an array of delegates, which are returned from global functions, like:
>> ---
>> struct Transformer
>> {
>> 	real delegate(real)[] funs;
>>
>> 	addFun(real x)
>> 	{
>> 		fun ~= makeFun(x);
>> 	}
>>
>> 	// etc.
>> }
>>
>> real delegate(real) makeFun(real x)
>> {
>> 	return (real r) => r * x;
>> }
>> ---
>>
>> This means my design was bad in the first place.
>> Thanks for the explanation.
>
> Actually, the delegate there is fine! The makeFun function becomes a closure, and will be allocated on the heap.
>
> Where you are running into trouble is simply that the struct goes out of scope, and the array is therefore invalid.
>
> In fact, I think you were already doing that before (creating a closure).
>
> Here is a possible solution to your original example:
>
> 	auto applyTo(T)(T list)
> 	{
> 		import std.algorithm;
>                 auto funcopy = fun;
> 		return list.map!(x => funcopy(x));
> 	}
>
> What's happening here is that funcopy is a stack local variable. However, since you're creating a delegate that uses local variables, the compiler creates a closure. In essence, it's like putting a new struct on the heap with the single member funcopy, and using that as the context pointer. Note that the original code also creates a closure, but 'fun' is a member of the hidden 'this' reference. Because the 'this' reference refers to destructed data, fun is garbage, hence the segfault.
>
> I actually was wrong about my original diagnosis. The delegate stored in your original code does NOT store a delegate with a context pointer that points to 'this', it's pointing to a closure. Because 'x' doesn't exist inside the struct, only inside the function. But my statements were still good advice, don't store pointers to yourself inside a struct :)
>
> -Steve

This makes perfect sense. My real code works as expected now.
Thanks for the clear explanation, and advice.

Nicolas