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

Richard (Rikki) Andrew Cattermole <alphaglosined@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |alphaglosined@gmail.com
         Resolution|---                         |WONTFIX

--- Comment #1 from Richard (Rikki) Andrew Cattermole <alphaglosined@gmail.com> ---
This is a misunderstanding of what ``@nogc`` is.

It is a linting tool. It guarantees that no GC allocation will take place within the function.

The reason why the compiler is not putting the closure on the stack is because it could escape the call stack. Mark the delegate parameter in ``acceptsCallback`` as ``scope`` and this will work.

Without the escaping guarantees, using the GC is the correct solution in this situation, and therefore should error.

--

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

--- Comment #2 from Manu <turkeyman@gmail.com> ---
Please think that through.

--

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

Tim <tim.dlang@t-online.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tim.dlang@t-online.de

--- Comment #3 from Tim <tim.dlang@t-online.de> ---
This is independent of @nogc. Code not marked @nogc would also benefit from this.

--

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

--- Comment #4 from Manu <turkeyman@gmail.com> ---
Correct, all code will benefit.
I just made the point because it's just particularly important for @nogc code,
where it is currently impossible to implement event-based systems conveniently.

--

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

--- Comment #5 from anonymous4 <dfj1esp02@sneakemail.com> ---
Lowering could be something like
---
struct Context
{
        T* ref_capture; //normal capture
}

void Closure(Context* ctx)
{
        bool oneref=false; //change this
        T* local_ref;
        if(oneref)local_ref=cast(T*)ctx;
        else local_ref=ctx.ref_capture;
        //call
        local_ref.method();
}
---

If one reference is captured, `oneref` variable could be changed(?) accordingly, and argument would be different, then backend will optimize it.

--

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

timon.gehr@gmx.ch changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
                 CC|                            |timon.gehr@gmx.ch
         Resolution|WONTFIX                     |---

--- Comment #6 from timon.gehr@gmx.ch ---
This is a valid enhancement request.

--

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

Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla@digitalmars.com

--- Comment #7 from Walter Bright <bugzilla@digitalmars.com> ---
Let's rewrite:

  class MyClass
  {
    void doSomething();

    void myMethod() @nogc
    {
        acceptsCallback((){ doSomething(); });
    }
  }

to make visible what is happening.

  class MyClass
  {
    void doSomething(MyClass this);

    void myMethod(MyClass this) @nogc
    {
        auto outer = &this;
        void func(MyClass* outer)
        {
            doSomething(*outer);
        }
        acceptsCallback(&outer.func);
    }
  }

func is a member of myMethod, not a member of MyClass. func's "this" pointer is a reference to the stack frame of myMethod, not a reference to MyClass. acceptsCallback() is allowed to squirrel away the address of func(), and could access it after myMethod() has exited. This will result in corrupted memory.

The solution is for acceptsCallback to annotate its parameter with `scope`, which says it will not preserve its argument past the lifetime of acceptsCallback.

--

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

--- Comment #8 from Manu <turkeyman@gmail.com> ---
I think you've completely missed the point...

Yes, me code DOES squirrel away the address of func, that is literally the point of this bug report.

The point is, the lambda doesn't access anything at all from the closure except 'this', and so rather than creating a closure like:

struct Closure
{
  MyClass this;
}
void func(Closure* closure) { closure.this.doSomething(); }

The closure is pointlessly allocated; there's no reason to synthesise the function this way. If the Closure ONLY references `MyClass this`, then func has no reason to allocate a closure and bind it to the delegate; it could just synthesise this function:

func(MyClass this) { this.doSomething(); }

This bypasses the closure since it has no reason to exist, and just places `this` into the delegate, and so for all intents and purposes, the lambda is just a normal method.

This optimisation is similarly valid for any closure which closes over exactly one pointer. It could even further be generalised such that any closure which closes over exactly a single size_t-sized POD just write that value to the 'this' pointer of the delegate (essentially small-struct optimisation embedded in the delegate), and then the lambda naturally receives it as first argument when the delegate it called.

There's a quite broad category of optimisation here; it's inefficient and pointless in most cases for a closure to be allocated with just one member... and it also has the nice advantage that in cases where a delegate has only a single reference (it, 'this'), then it doesn't need to allocate a closure at all, and so it's naturally `@nogc`.

--

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

Richard (Rikki) Andrew Cattermole <alphaglosined@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|@nogc lambda shouldn't      |A closure with a layout of
                   |allocate closure when       |pointer size or below that
                   |lambda refers only to       |is not modified, should not
                   |'this'                      |have a closure

--

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

--- Comment #9 from Richard (Rikki) Andrew Cattermole <alphaglosined@gmail.com> ---
Okay now it starts to make sense what you are wanting.

If the closure's layout is a pointer in size or less you do not want it to exist.

It does require that this value is not modified, although what a pointer points to may be modified (head const).

That leads me to the question for Walter, how much in the frontend needs to change for this optimization to be implemented by ldc/gdc?

If it does not need to be changed, then this should be viewed as a ldc/gdc bug not a dmd one (which you shouldn't be using when optimizations are needed).

--