On 01/06/2014 11:13 AM, Vladimir Panteleev wrote:
>
> IMO, the effort to disallow third parties to "emit" signals on your
> object by far exceeds its worth.
>
> For this purpose, you have needed:
> - string mixins
>    - ...which declare, by convention, a field with a name not explicitly
> given by the user
>    - ...with a workaround for when they don't work
> - an enumeration type
> - two different signal struct types
> - a boatload of documentation
> - a nontrivial cognitive effort on the user to grok all of the above
>
> This little feature steals too much focus, and IMO is not worth it.

I absolutely agree. Ideally the module std.signal would consists of a template struct with 3 overloaded methods.

struct Signal(Args...)
{
    void connect(void delegate(Args) dg);
    void connect(WeakDelegate!(Args) dg);

    void disconnect(void delegate(Args) dg);
    void disconnect(WeakDelegate!(Args) dg);

    void emit(Args);
}

Still, looking at the documentation (https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html) and implementation you're somewhat off.

Of course there are a few implementation issues.
- There is no WeakDelegate in druntime or phobos.

  Maybe requiring explicit disconnect is good enough?

- The lifetime of the delegate context could be scoped.

  No idea how to solve this one, but you could require that
  the delegate context is on the GC heap.

- When used this Signal definition requires boilerplate
  to restrict access to emit.

  This is unlucky but doesn't justify doubling the complexity
  of the module.

-Martin

On Thursday, 23 January 2014 at 16:17:38 UTC, Martin Nowak wrote:
> On 01/06/2014 11:39 AM, Dicebot wrote:
>> =/ Any good idea to make those more visible? Maybe we should setup a
>> mailing list for those who want to be notified about ongoing reviews?
>
> Yes, a separate newsgroup would help here.
> Do you have an idea for a name?
> digitalmars.D.phobos or digitalmars.D.review

digitalmars.D.review is better. We only review Phobos module that way now but it can be extended.

Related offtopic:

I wonder sometimes if it can/should be generalized for any important decision making involving community. Currently only Phobos reviews live with their own somewhat independent life, everything else is process bottleneck mandating some intervention from small core team. Probably investing into some tooling here can result in a great benefit.

On 2014-01-23 19:26, Dicebot wrote:

> digitalmars.D.review is better. We only review Phobos module that way
> now but it can be extended.
>
> Related offtopic:
>
> I wonder sometimes if it can/should be generalized for any important
> decision making involving community. Currently only Phobos reviews live
> with their own somewhat independent life, everything else is process
> bottleneck mandating some intervention from small core team. Probably
> investing into some tooling here can result in a great benefit.

I agree. There are also many DIP's which are just sitting in the wiki.

-- 
/Jacob Carlborg

> (https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html)

Was outdated (web/phobos-prerelase/std_signal.html was current), but the old link you posted is now also up2date. Sorry for that.

Best regards,

Robert

On 01/23/2014 07:26 PM, Dicebot wrote:
> I wonder sometimes if it can/should be generalized for any important
> decision making involving community. Currently only Phobos reviews live
> with their own somewhat independent life, everything else is process
> bottleneck mandating some intervention from small core team. Probably
> investing into some tooling here can result in a great benefit.

I would have really like more thorough review for the shared library pull request, although I'd like more code review in general.
https://github.com/D-Programming-Language/druntime/pull/617

On 01/11/2014 08:13 PM, Robert wrote:
>>
>> I agree. Is the only reason to have a weak connection?
>
> Yes. Weak connections are the only reason.

Wouldn't it be possible to find out whether the delegate context ptr
is actually an object? Not sure how to do it safely though and Interfaces slightly differ.

```d
import std.stdio;

class Foo
{
    void method() {}
}

void main()
{
    auto foo = new Foo;
    auto dg = &foo.method;
    // It's a class delegate!!!
    writeln(cast(void*)foo.classinfo.__vptr, " ", ***cast(void****)dg.ptr, " ", (new ClassInfo).__vptr);
    assert(***cast(void****)dg.ptr is (new ClassInfo).__vptr);
}
```

On 01/23/2014 06:16 PM, Martin Nowak wrote:
> Of course there are a few implementation issues.
> - There is no WeakDelegate in druntime or phobos.
>
>    Maybe requiring explicit disconnect is good enough?
>
> - The lifetime of the delegate context could be scoped.
>
>    No idea how to solve this one, but you could require that
>    the delegate context is on the GC heap.
>
> - When used this Signal definition requires boilerplate
>    to restrict access to emit.
>
>    This is unlucky but doesn't justify doubling the complexity
>    of the module.

BTW, this is the reason why I didn't participate in the review.
We need to think harder about solving these issues without degrading the API. I am simply lacking the time to get into this, but I'm sure
that a better trade-off can be found.

On 01/13/2014 10:16 PM, ilya-stromberg wrote:
>
> It's not so good to have array of delegates because you will have a
> memory leaks. Delegate has permanent pointer to the object, so GC will
> never free it. As alternative, you can delete delegate manually, but in
> this case you can have problems with manual memory management.
>
> So, array of delegates is possible solution, but only for VERY simple
> cases. This module solves problem with memory leaks because it
> implements weak connections (based on weak reference). It's easy to use,
> but VERY difficult to implement.

But weak references should be implemented on GC/druntime level.

On Thursday, 23 January 2014 at 22:58:08 UTC, Martin Nowak wrote:
> On 01/13/2014 10:16 PM, ilya-stromberg wrote:
>>
>> It's not so good to have array of delegates because you will have a
>> memory leaks. Delegate has permanent pointer to the object, so GC will
>> never free it. As alternative, you can delete delegate manually, but in
>> this case you can have problems with manual memory management.
>>
>> So, array of delegates is possible solution, but only for VERY simple
>> cases. This module solves problem with memory leaks because it
>> implements weak connections (based on weak reference). It's easy to use,
>> but VERY difficult to implement.
>
> But weak references should be implemented on GC/druntime level.

Yes, I know. They can be useful for many other purposes. I just provide example when `std.signal` better than `array of delegates`.

On 2014-01-23 23:51, Martin Nowak wrote:

> Wouldn't it be possible to find out whether the delegate context ptr
> is actually an object? Not sure how to do it safely though and
> Interfaces slightly differ.
>
> ```d
> import std.stdio;
>
> class Foo
> {
>      void method() {}
> }
>
> void main()
> {
>      auto foo = new Foo;
>      auto dg = &foo.method;
>      // It's a class delegate!!!
>      writeln(cast(void*)foo.classinfo.__vptr, " ",
> ***cast(void****)dg.ptr, " ", (new ClassInfo).__vptr);
>      assert(***cast(void****)dg.ptr is (new ClassInfo).__vptr);
> }
> ```

Why don't you just cast the delegate context pointer to Object? Like this:

auto result = cast(Object) dg.ptr;

If "result" is not null the context pointer points to an object. Although this won't handled interfaces. I consider it a bug that an interface cannot be casted to Object.

-- 
/Jacob Carlborg