Walter Bright wrote:
> Walter Bright wrote:
>> http://www.digitalmars.com/d/std_signals.html
>>
>> And that, hopefully, is that.
> 
> Obviously not...
> 
> Here's one incorporating the suggested improvements:
> 
> http://www.digitalmars.com/d/phobos/signal.d

That link doesn't resolve.

Sean Kelly wrote:
> Walter Bright wrote:
>> Walter Bright wrote:
>>> http://www.digitalmars.com/d/std_signals.html
>>>
>>> And that, hopefully, is that.
>>
>> Obviously not...
>>
>> Here's one incorporating the suggested improvements:
>>
>> http://www.digitalmars.com/d/phobos/signal.d
> 
> That link doesn't resolve.

http://www.digitalmars.com/d/phobos/signals.d  (with an 's')

Walter Bright wrote:

> Chris Miller wrote:
>> I think this is finally a real mixin limitation being exposed. You probably only tested std.signals inside the signals.d source file, where the mixin had access to std.signals' imports.
> 
> I plead guilty. My test suite is now corrected.
> 
>> But use std.signals from
>> another file and the mixin cannot access std.signal's imports because
>> it's accessing the mixed-in scope. In other words, I think to remove
>> these errors, std.signals' imports would need to be imported inside the
>> mixin template (hack?), or change how mixins work.
> 
> The fix is straightforward, in std\signals.d just make the imports public:
> 
>> public import std.stdio;
>> public import std.c.stdlib;
>> public import std.outofmemory;

There were some reasons for keeping imports private, you know. I believe there were some discussions too ;)

-- 
Lars Ivar Igesund
blog at http://larsivi.net
DSource & #D: larsivi

Some more suggestions, interspersed in the source:

> // Special function for internal use only.
> // Use of this is where the slot had better be a delegate
> // to an object or an interface that is part of an object.
> extern (C) Object _d_toObject(void* p);

If it's for internal use only, why isn't it private?

[...]
> template Signal(T1...)
> {
>     /***
>      * A slot is implemented as a delegate.
>      * The slot_t is the type of the delegate.
>      * The delegate must be to an instance of a class or an interface
>      * to a class instance.
>      * Delegates to struct instances or nested functions must not be
>      * used as slots.
>      */
>     alias void delegate(T1) slot_t;
>
>     /***
>      * Call each of the connected slots, passing the argument(s) i to them.
>      */
>     void emit( T1 i )
>     {
>         foreach (slot; slots)

No need to iterate over the *entire* array, this should do:
         foreach (slot; slots[0 .. slots_idx])

>         {   if (slot)

If my other modifications are implemented, this check will be unnecessary

>                 slot(i);
>         }
>     }
>
>     /***
>      * Add a slot to the list of slots to be called when emit() is called.
>      */
>     void connect(slot_t slot)
>     {
>         /* Do this:
>          *    slots ~= slot;
>          * but use malloc() and friends instead
>          */
>         auto len = slots.length;
>         if (slots_idx == len)
>         {
>             if (slots.length == 0)
>             {
>                 len = 4;
>                 auto p = std.signals.calloc(slot_t.sizeof, len);
>                 if (!p)
>                     std.signals._d_OutOfMemory();
>                 slots = (cast(slot_t*)p)[0 .. len];
>             }
>             else
>             {


>                 // Look for unused entry in slots[]
>                 foreach (inout s; slots)
>                 {
>                     if (!s)                // found unused entry
>                     {        s = slot;
>                         goto L1;
>                     }
>                 }

Unnecessary O(n) operation that can be avoided by a little extra work in
disconnect() and unhook(), see below.

>
>                 len = len * 2 + 4;
>                 auto p = std.signals.realloc(slots.ptr, slot_t.sizeof * len);
>                 if (!p)
>                     std.signals._d_OutOfMemory();
>                 slots = (cast(slot_t*)p)[0 .. len];
>                 slots[slots_idx + 1 .. length] = null;
>             }
>         }
>         slots[slots_idx++] = slot;
>
>      L1:
>         Object o = _d_toObject(slot.ptr);
>         o.notifyRegister(&unhook);
>     }
>
>     /***
>      * Remove a slot from the list of slots to be called when emit() is called.
>      */
>     void disconnect( slot_t slot)
>     {
>         debug (signal) writefln("Signal.disconnect(slot)");
>         foreach (inout dg; slots)

        foreach (inout dg; slots[0 .. slots_idx])

>         {
>             if (dg == slot)

>             {   dg = null;

If we replace this line by:
              {  slots_idx--;
                 dg = slots[slots_index];
                 slots[slots_idx] = null;
then we don't need to check for empty slots in emit():
If slots.length == slots_idx, we can be sure the array is full.

(Unless there's a requirement for the slots to be
called in any particular order?)

>
>                 Object o = _d_toObject(slot.ptr);
>                 o.notifyUnRegister(&unhook);
>             }
>         }
>     }
>
>     /* **
>      * Special function called when o is destroyed.
>      * It causes any slots dependent on o to be removed from the list
>      * of slots to be called by emit().
>      */
>     void unhook(Object o)
>     {
>         debug (signal) writefln("Signal.unhook(o = %s)", cast(void*)o);
>         foreach (inout slot; slots)

        foreach (inout slot; slots[0 .. slots_idx])

>         {

>             if (slot.ptr is o)
>                 slot = null;

Besides shrinking slots[0 .. slots_idx] to match, I have another remark here.
If slot is an interface delegate, this doesn't work. The test should probably be changed to:
            if (_d_toObject(slot.ptr) is o)
to handle this case.

>         }
>     }
>
>     /* **
>      * There can be multiple destructors inserted by mixins.
>      */
>     ~this()
>     {
>         /* **
>          * When this object is destroyed, need to let every slot
>          * know that this object is destroyed so they are not left
>          * with dangling references to it.
>          */
>         if (slots)
>         {
>             foreach (slot; slots)

            foreach (slot; slots[0 .. slots_idx])

>             {
>                 if (slot)
>                 {   Object o = _d_toObject(slot.ptr);
>                     o.notifyUnRegister(&unhook);
>                 }
>             }
>             std.signals.free(slots.ptr);
>             slots = null;
>         }
>     }
>
>   private:
>     slot_t[] slots;                // the slots to call from emit()
>     size_t slots_idx;                // used length of slots[]
> }

Walter Bright wrote:
> http://www.digitalmars.com/d/std_signals.html
> 
> And that, hopefully, is that.

Is there any plans to support return values? This is much prettier compared to having an extra out parameter.

Consider a GUI. Sometimes you need to know when the event has been used and processing should stop...

Regarding the mixin issue I'm not sure what I think yet. A new 'scope(instance) / scope(module)' keeps coming up in my head though.

All I want to mixin is the public interface, not the implementation!

Frits van Bommel wrote:
> 
> If we replace this line by:
>               {  slots_idx--;
>                  dg = slots[slots_index];
>                  slots[slots_idx] = null;
> then we don't need to check for empty slots in emit():
> If slots.length == slots_idx, we can be sure the array is full.
> 
> (Unless there's a requirement for the slots to be
> called in any particular order?)

No, but the sequence may need to be modified while emit is processing. Consider a slot that disconnects itself when it receives a signal.  But I suppose you could do something like this:

void emit( T1 i )
{
    {
        isEmitting = true;
        scope(exit) isEmitting = false;

        foreach( s; slots )
            s(i);
    }
    foreach( s; disconnects )
        disconnect( s );
}

void disconnect( slot_t s )
{
    if( !isEmitting )
    {
        // remove from slots
        return;
    }
    disconnects ~= s;
}

Things get even worse in a multithreaded app if the signal is shared and other threads can be connecting and disconnecting at any time.


Sean

Sean Kelly wrote:
> Frits van Bommel wrote:
>>
>> If we replace this line by:
>>               {  slots_idx--;
>>                  dg = slots[slots_index];
>>                  slots[slots_idx] = null;
>> then we don't need to check for empty slots in emit():
>> If slots.length == slots_idx, we can be sure the array is full.
>>
>> (Unless there's a requirement for the slots to be
>> called in any particular order?)
> 
> No, but the sequence may need to be modified while emit is processing. Consider a slot that disconnects itself when it receives a signal.  But 

Yeah, that can create some extra headaches, of course.

> I suppose you could do something like this:
> 
> void emit( T1 i )
> {
>     {
>         isEmitting = true;
>         scope(exit) isEmitting = false;
> 
>         foreach( s; slots )
>             s(i);
>     }
>     foreach( s; disconnects )
>         disconnect( s );

You may want to add
    disconnects.length = 0;
or
    disconnects = null;
here.

> }
> 
> void disconnect( slot_t s )
> {
>     if( !isEmitting )
>     {
>         // remove from slots
>         return;
>     }
>     disconnects ~= s;
> }
> 
> Things get even worse in a multithreaded app [...]

They always do ;)
Seriously, I think if your app is multithreading you're probably going to have to synchronize most things that can be accessed by multiple threads.
Since the original implementation was not thread-safe I saw no reason to make my modifications so.

Frits van Bommel wrote:
> Sean Kelly wrote:
>> Frits van Bommel wrote:
>>>
>>> If we replace this line by:
>>>               {  slots_idx--;
>>>                  dg = slots[slots_index];
>>>                  slots[slots_idx] = null;
>>> then we don't need to check for empty slots in emit():
>>> If slots.length == slots_idx, we can be sure the array is full.
>>>
>>> (Unless there's a requirement for the slots to be
>>> called in any particular order?)
>>
>> No, but the sequence may need to be modified while emit is processing. Consider a slot that disconnects itself when it receives a signal.  But 
> 
> Yeah, that can create some extra headaches, of course.

By the way, I only mentioned this because of the foreach restrictions on modifying a sequence while it is being processed.  If this restriction were not present, your example would be fine as-is.  There's also probably a better way to handle this situation than what I outlined--it was just an example :-)


Sean

Tom S wrote:
> Walter Bright wrote:
> 
>> The fix is straightforward, in std\signals.d just make the imports public:
>>
>>> public import std.stdio;
>>> public import std.c.stdlib;
>>> public import std.outofmemory;
> 
> 
> Sorry Walter, but the name for it is 'hack', not 'fix' :( How about fixing mixins ?

if(hack == "get it to compile until I can make a real fix")
	writef("good\n");
else
	writef("BAD\n");

Sean Kelly wrote:
> Frits van Bommel wrote:
>> Sean Kelly wrote:
>>> Frits van Bommel wrote:
>>>>
>>>> If we replace this line by:
>>>>               {  slots_idx--;
>>>>                  dg = slots[slots_index];
>>>>                  slots[slots_idx] = null;
>>>> then we don't need to check for empty slots in emit():
>>>> If slots.length == slots_idx, we can be sure the array is full.
>>>>
>>>> (Unless there's a requirement for the slots to be
>>>> called in any particular order?)
>>>
>>> No, but the sequence may need to be modified while emit is processing. Consider a slot that disconnects itself when it receives a signal.  But 
>>
>> Yeah, that can create some extra headaches, of course.
> 
> By the way, I only mentioned this because of the foreach restrictions on modifying a sequence while it is being processed.  If this restriction were not present, your example would be fine as-is.  There's also 

No it wouldn't (be fine). Even if that restriction was not in place, it'd probably still skip the last slot if the current slot removed itself (since it would be put into a place in the array the loop considered to be done).
Of course, this could be fixed by checking whether the slot was modified after it returns, and rerunning it if it was. Preferably by decrementing the loop counter, so it'd still work if the next slot does the same or the current slot _was_ the last slot.
You'd need to use a for-loop for that though, not foreach. (to have a modifiable index, and because the end of the sequence can change)