www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Signals and Slots

reply Walter Bright <newshound digitalmars.com> writes:
http://www.digitalmars.com/d/std_signals.html

And that, hopefully, is that.
Nov 02 2006
next sibling parent =?ISO-8859-1?Q?Anders_F_Bj=F6rklund?= <afb algonet.se> writes:
Walter Bright wrote:


 http://www.digitalmars.com/d/std_signals.html
 
 And that, hopefully, is that.


Almost that...

http://www.digitalmars.com/d/phobos/std_signals.html

--anders
Nov 02 2006
prev sibling next sibling parent reply Lutger <lutger.blijdestijn gmail.com> writes:
Walter Bright wrote:

 http://www.digitalmars.com/d/std_signals.html
 
 And that, hopefully, is that.


Does the following mean you intend to make signals compatible with other 
callable types?

"BUGS:
Slots can only be delegates formed from class objects or interfaces to 
class objects."
Nov 02 2006
parent Walter Bright <newshound digitalmars.com> writes:
Lutger wrote:

 Does the following mean you intend to make signals compatible with other 
 callable types?
 
 "BUGS:
 Slots can only be delegates formed from class objects or interfaces to 
 class objects."


Not for the time being. The problem is the type system cannot 
distinguish them.
Nov 02 2006
prev sibling next sibling parent reply "Chris Miller" <chris dprogramming.com> writes:
Using the example straight from  
http://www.digitalmars.com/d/phobos/std_signals.html (and yes using DMD  
0.173) I get the following compiler errors:


c:\dmd\bin\..\src\phobos\std\signals.d(166): undefined identifier  
_d_OutOfMemory

c:\dmd\bin\..\src\phobos\std\signals.d(166): function expected before (),  
not _d
_OutOfMemory of type int
c:\dmd\bin\..\src\phobos\std\signals.d(174): undefined identifier  
_d_OutOfMemory

c:\dmd\bin\..\src\phobos\std\signals.d(174): function expected before (),  
not _d
_OutOfMemory of type int
c:\dmd\bin\..\src\phobos\std\signals.d(235): undefined identifier free
c:\dmd\bin\..\src\phobos\std\signals.d(235): function expected before (),  
not fr
ee of type int


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. 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.
Nov 02 2006
next sibling parent Lars Ivar Igesund <larsivar igesund.net> writes:
Chris Miller wrote:


 Using the example straight from
 http://www.digitalmars.com/d/phobos/std_signals.html (and yes using DMD
 0.173) I get the following compiler errors:
 
 
 c:\dmd\bin\..\src\phobos\std\signals.d(166): undefined identifier
 _d_OutOfMemory
 
 c:\dmd\bin\..\src\phobos\std\signals.d(166): function expected before (),
 not _d
 _OutOfMemory of type int
 c:\dmd\bin\..\src\phobos\std\signals.d(174): undefined identifier
 _d_OutOfMemory
 
 c:\dmd\bin\..\src\phobos\std\signals.d(174): function expected before (),
 not _d
 _OutOfMemory of type int
 c:\dmd\bin\..\src\phobos\std\signals.d(235): undefined identifier free
 c:\dmd\bin\..\src\phobos\std\signals.d(235): function expected before (),
 not fr
 ee of type int
 
 
 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. 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.


Maybe this is the que for Walter to finally accept that mixins are far from
perfect as they are? Buggy and not optimally designed.

-- 
Lars Ivar Igesund
blog at http://larsivi.net
DSource & #D: larsivi
Nov 02 2006
prev sibling next sibling parent Tom S <h3r3tic remove.mat.uni.torun.pl> writes:
Chris Miller wrote:

 Using the example straight from 
 http://www.digitalmars.com/d/phobos/std_signals.html (and yes using DMD 
 0.173) I get the following compiler errors:
 
 
 c:\dmd\bin\..\src\phobos\std\signals.d(166): undefined identifier 
 _d_OutOfMemory
 
 c:\dmd\bin\..\src\phobos\std\signals.d(166): function expected before 
 (), not _d
 _OutOfMemory of type int
 c:\dmd\bin\..\src\phobos\std\signals.d(174): undefined identifier 
 _d_OutOfMemory
 
 c:\dmd\bin\..\src\phobos\std\signals.d(174): function expected before 
 (), not _d
 _OutOfMemory of type int
 c:\dmd\bin\..\src\phobos\std\signals.d(235): undefined identifier free
 c:\dmd\bin\..\src\phobos\std\signals.d(235): function expected before 
 (), not fr
 ee of type int
 
 
 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. 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.


++mixinVictims;

Thanks for bringing up that problem, Chris. I've begged for a change in 
mixins' visibility rules some time ago. Maybe we could restart the 
discussion now ?

Mixins are unlike any other construct in D, thus they are a trap for 
programmers expecting uniform behavior in the whole language.
The biggest problem with mixins is what has just been demonstrated by 
Chris - they exist in instantiation scope and that's where they look for 
symbols. But when we write a module with some template meant to be used 
as a mixin, the module will probably contain other stuff and we'll want 
our mixin to use it. So we use that symbol in the mixin and it works in 
the module... But then we import the module somewhere else, try to use 
the mixin and BANG! Lots of 'undefined identifier' errors.

To be consistent, mixins should by default see their declaration scope 
and only access the instantiation scope thru a special keyword or 
construct. If it were up to me, I'd use 'outer'. E.g.

template Foo() {
	alias foo a;	// uses 'foo' from the declaration scope
	alias outer.foo b;	// uses 'foo' from the instantiation scope
}

That's all is needed to avoid unpleasant surprises with mixin visibility 
rules.

On a last note, this is not the only problem with mixins. I hope that we 
can finally get around to fixing them all...

--
Tomasz Stachowiak
Nov 02 2006
prev sibling parent reply Walter Bright <newshound digitalmars.com> writes:
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;
Nov 02 2006
next sibling parent Chris Nicholson-Sauls <ibisbasenji gmail.com> writes:
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;




Maybe instead, it should be something like this?




These are modules that user modules are plenty likely not to want in their
namespace, so 
it would be good to put a little indirection like this in place.  Just two
cents.

-- Chris Nicholson-Sauls
Nov 02 2006
prev sibling next sibling parent reply "Jarrett Billingsley" <kb3ctd2 yahoo.com> writes:
"Walter Bright" <newshound digitalmars.com> wrote in message 
news:eidh0g$10e1$1 digitaldaemon.com...


 public import std.stdio;
 public import std.c.stdlib;
 public import std.outofmemory;




That then works when the class has one signal, but trying to put more than 
one:

class Input
{
    mixin Signal!(int, int) click;
    mixin Signal!(char) keyDown;
}

Gives..

C:\dmd\bin\..\src\phobos\std\signals.d(181): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(196): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(232): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(181): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(196): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)
C:\dmd\bin\..\src\phobos\std\signals.d(232): Error: 
dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
dtest.Input.Signal!(char).unhook at dtest.d(206)

 Execution finished.


:S

Mixins, mixins, mixins.
Nov 02 2006
parent reply Dave <Dave_member pathlink.com> writes:
Jarrett Billingsley wrote:

 "Walter Bright" <newshound digitalmars.com> wrote in message 
 news:eidh0g$10e1$1 digitaldaemon.com...
 

 public import std.stdio;
 public import std.c.stdlib;
 public import std.outofmemory;




 
 That then works when the class has one signal, but trying to put more than 
 one:
 
 class Input
 {
     mixin Signal!(int, int) click;
     mixin Signal!(char) keyDown;
 }
 


Good catch - this also happens w/o the 'public import' mod. in std/signals (if
you import those into 
your module) as well.

This is probably a show-stopper, because I don't think this would compete with
the current QT 
signal/slot mechanism, for example, if we have to wrap each Signal with a
separate class.

Walter - is there a work-around for this problem, or a 'fix' of some sort
planned?

I mean this is very, very cool... QT only recently bowed to market demand for a
port to Java (their 
website used to complain of Java performance issues), and the MOC hack has
always been a problem for 
C++ (IMHO). D is a natural for QT if this can be fixed. I think if it can be
fixed, you should 
approach them with it. It would be huge for D (and potentially for QT as well).

Thanks for these awesome new features!


 Gives..
 
 C:\dmd\bin\..\src\phobos\std\signals.d(181): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)
 C:\dmd\bin\..\src\phobos\std\signals.d(196): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)
 C:\dmd\bin\..\src\phobos\std\signals.d(232): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)
 C:\dmd\bin\..\src\phobos\std\signals.d(181): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)
 C:\dmd\bin\..\src\phobos\std\signals.d(196): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)
 C:\dmd\bin\..\src\phobos\std\signals.d(232): Error: 
 dtest.Input.Signal!(int,int).unhook at dtest.d(206) conflicts with 
 dtest.Input.Signal!(char).unhook at dtest.d(206)

 Execution finished.


 
 :S
 
 Mixins, mixins, mixins.
Nov 06 2006
parent reply Walter Bright <newshound digitalmars.com> writes:
Dave wrote:

 Walter - is there a work-around for this problem, or a 'fix' of some 
 sort planned?


It's already fixed.
Nov 06 2006
next sibling parent Dave <Dave_member pathlink.com> writes:
Walter Bright wrote:

 Dave wrote:

 Walter - is there a work-around for this problem, or a 'fix' of some 
 sort planned?


 
 It's already fixed.


Even more cool!
Nov 06 2006
prev sibling parent reply Bill Baxter <wbaxter gmail.com> writes:
Walter Bright wrote:

 Dave wrote:
 

 Walter - is there a work-around for this problem, or a 'fix' of some 
 sort planned?


 
 
 It's already fixed.


Is this a fix we can make ourselves ourselves by tinkering with 
std/signals.d?  I was going to start playing with std.signals, to see 
how it stacked up against my current code using Lutger's signal library. 
  But it's a non-starter without being able to have multiple signals in 
one class.

--bb
Nov 10 2006
parent reply Walter Bright <newshound digitalmars.com> writes:
Bill Baxter wrote:

 Is this a fix we can make ourselves ourselves by tinkering with 
 std/signals.d?


http://www.digitalmars.com/d/phobos/signal.d
Nov 11 2006
next sibling parent Bill Baxter <wbaxter gmail.com> writes:
Walter Bright wrote:

 Bill Baxter wrote:
 

 Is this a fix we can make ourselves ourselves by tinkering with 
 std/signals.d?


 
 
 http://www.digitalmars.com/d/phobos/signal.d


Ok.  Got it.  Thanks.

But it's
http://www.digitalmars.com/d/phobos/signals.d  << note the 's'


--bb
Nov 11 2006
prev sibling parent reply Bill Baxter <dnewsgroup billbaxter.com> writes:
Walter Bright wrote:

 Bill Baxter wrote:

 Is this a fix we can make ourselves ourselves by tinkering with 
 std/signals.d?


 
 http://www.digitalmars.com/d/phobos/signal.d



Here's a few more problems with std.signals:

*) Can't make a global signal.  With Lutger's versions using objects for 
signals this wasn't a problem.  But a mixin in the global scope causes a 
"can't have a dtor outside a class" error.

If you stick with mixins for implementing signals, I think there should 
probabaly be a simple SignalObj class that wraps a signal. Unfortunately 
   pretty much anything I've tried along these lines starting with

      class SignalObj(T...)
      {
      }

currently generates an ICE.


*) More mixin import scoping issues.
    This seems to be impossible:

    import ssig=std.signals;

    And that isn't fixed by making the std/signals.d imports public.

--bb
Nov 13 2006
parent reply Walter Bright <newshound digitalmars.com> writes:
Bill Baxter wrote:

 *) Can't make a global signal.  With Lutger's versions using objects for 
 signals this wasn't a problem.  But a mixin in the global scope causes a 
 "can't have a dtor outside a class" error.


Why not just put it in a global class instance?


 If you stick with mixins for implementing signals, I think there should 
 probabaly be a simple SignalObj class that wraps a signal. Unfortunately 
   pretty much anything I've tried along these lines starting with
 
      class SignalObj(T...)
      {
      }
 
 currently generates an ICE.


Please post all examples that generate an ICE to bugzilla.
Nov 13 2006
parent reply Bill Baxter <wbaxter gmail.com> writes:
Walter Bright wrote:

 Bill Baxter wrote:
 

 *) Can't make a global signal.  With Lutger's versions using objects 
 for signals this wasn't a problem.  But a mixin in the global scope 
 causes a "can't have a dtor outside a class" error.


 
 
 Why not just put it in a global class instance?
 

 If you stick with mixins for implementing signals, I think there 
 should probabaly be a simple SignalObj class that wraps a signal. 
 Unfortunately   pretty much anything I've tried along these lines 
 starting with

      class SignalObj(T...)
      {
      }

 currently generates an ICE.


 
 
 Please post all examples that generate an ICE to bugzilla.


I think the one I was hitting was the same as this one I posted with a 
slightly different description:
    http://d.puremagic.com/issues/show_bug.cgi?id=495

But I went ahead and added the class SignalObj(T...) variant of it. 
Both make
   Assertion failure: 'global.errors' on line 2752 in file 'template.c'

--bb
Nov 13 2006
parent Walter Bright <newshound digitalmars.com> writes:
Thank-you.
Nov 13 2006
prev sibling next sibling parent Kyle Furlong <kylefurlong gmail.com> writes:
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;




Walter, please don't pull a GW here.
Nov 02 2006
prev sibling next sibling parent "Chris Miller" <chris dprogramming.com> writes:
On Thu, 02 Nov 2006 14:30:25 -0500, Walter Bright  
<newshound digitalmars.com> 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;




Yuck. Did somebody say unprovoked import conflict? How about making all  
imports public! HOORAY
Nov 02 2006
prev sibling next sibling parent reply Tom S <h3r3tic remove.mat.uni.torun.pl> writes:
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 ?
Nov 02 2006
parent BCS <BCS pathlink.com> writes:
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");
Nov 03 2006
prev sibling parent Lars Ivar Igesund <larsivar igesund.net> writes:
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
Nov 03 2006
prev sibling next sibling parent reply Sean Kelly <sean f4.ca> writes:
Walter Bright wrote:

 http://www.digitalmars.com/d/std_signals.html
 
 And that, hopefully, is that.


Nice work!  In skimming the code, one thing jumped out at me:

signals.d:171: len += len + 4;

I think this should be: len += 4;

Or actually: len += (void*).sizeof;

To be 64-bit safe.  Finally, is there any reason to use _d_toObject in 
this code instead of cast(Object)?  I'd think they would do the same thing?


Sean
Nov 02 2006
parent reply Walter Bright <newshound digitalmars.com> writes:
Sean Kelly wrote:

 Walter Bright wrote:

 http://www.digitalmars.com/d/std_signals.html

 And that, hopefully, is that.


 
 Nice work!  In skimming the code, one thing jumped out at me:
 
 signals.d:171: len += len + 4;
 
 I think this should be: len += 4;
 
 Or actually: len += (void*).sizeof;


No, that is as intended. It's using a power of 2 allocation method. 
Also, the sizeof thing is taken care of by the call to calloc/realloc.


 To be 64-bit safe.  Finally, is there any reason to use _d_toObject in 
 this code instead of cast(Object)?  I'd think they would do the same thing?


Not exactly. cast(Object)p won't do the pointer adjustment if the type 
of p is a void*. To convert an interface to its underlying Object, the 
cast implementation needs to know what the interface type is.
Nov 02 2006
next sibling parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Walter Bright wrote:

 Sean Kelly wrote:

 Walter Bright wrote:

 http://www.digitalmars.com/d/std_signals.html

 And that, hopefully, is that.


 Nice work!  In skimming the code, one thing jumped out at me:

 signals.d:171: len += len + 4;

 I think this should be: len += 4;

 Or actually: len += (void*).sizeof;


 
 No, that is as intended. It's using a power of 2 allocation method. 
 Also, the sizeof thing is taken care of by the call to calloc/realloc.


Then it's massively over-allocating. Note that it performs a call to 
calloc/realloc *every single time* the function is called. It (more 
than) doubles the length and sets the last element to the new slots.
Nov 02 2006
parent Walter Bright <newshound digitalmars.com> writes:
Frits van Bommel wrote:

 Walter Bright wrote:

 No, that is as intended. It's using a power of 2 allocation method. 
 Also, the sizeof thing is taken care of by the call to calloc/realloc.


 
 Then it's massively over-allocating. Note that it performs a call to 
 calloc/realloc *every single time* the function is called. It (more 
 than) doubles the length and sets the last element to the new slots.


Looks like I screwed that up!
Nov 02 2006
prev sibling parent reply Sean Kelly <sean f4.ca> writes:
Walter Bright wrote:

 Sean Kelly wrote:

 Walter Bright wrote:

 http://www.digitalmars.com/d/std_signals.html

 And that, hopefully, is that.


 Nice work!  In skimming the code, one thing jumped out at me:

 signals.d:171: len += len + 4;

 I think this should be: len += 4;

 Or actually: len += (void*).sizeof;


 
 No, that is as intended. It's using a power of 2 allocation method.


Okay, now I'm confused.  Here's the pertinent bit of code, edited for 
clarity:

     auto len = slots.length;
     auto startlen = len;

     len += len + 4;
     auto p = std.c.stdlib.realloc(slots.ptr, slot_t.sizeof * len);
     slots = (cast(slot_t*)p)[0 .. len];
     slots[startlen] = slot;

This looks like the allocated buffer will never be more than half full, 
because a reallocation of size len * 2 occurs every time connect is 
called.  This should give buffer sizes of: 4, 8, 20, 44, 92, etc, with 
the new element added at 0, 4, 8, 20, and 44, respectively.  In fact, it 
seems the buffer density will decrease over time because the space 
between elements doubles every insertion?  Hrm, perhaps this line:

     slots = (cast(slot_t*)p)[0 .. len];

should be more like this:

     slots = (cast(slot_t*)p)[0 .. startlen + 1];

so the next call to slots.length isn't aware of the unused space?


 To be 64-bit safe.  Finally, is there any reason to use _d_toObject in 
 this code instead of cast(Object)?  I'd think they would do the same 
 thing?


 
 Not exactly. cast(Object)p won't do the pointer adjustment if the type 
 of p is a void*. To convert an interface to its underlying Object, the 
 cast implementation needs to know what the interface type is.


Oh I see, so this is because p can be an interface as well.  Thanks.


Sean
Nov 02 2006
parent Sean Kelly <sean f4.ca> writes:
Sean Kelly wrote:

 Hrm, perhaps this line:
 
     slots = (cast(slot_t*)p)[0 .. len];
 
 should be more like this:
 
     slots = (cast(slot_t*)p)[0 .. startlen + 1];
 
 so the next call to slots.length isn't aware of the unused space?


On second thought, scratch this idea.  It would cause linear growth and 
half the buffer would still go unused.


Sean
Nov 02 2006
prev sibling next sibling parent reply Walter Bright <newshound digitalmars.com> writes:
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
Nov 02 2006
next sibling parent reply Sean Kelly <sean f4.ca> writes:
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.
Nov 02 2006
parent Walter Bright <newshound digitalmars.com> writes:
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')
Nov 02 2006
prev sibling parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
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[]
 }
Nov 03 2006
parent reply Sean Kelly <sean f4.ca> writes:
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
Nov 03 2006
parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
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.
Nov 03 2006
parent reply Sean Kelly <sean f4.ca> writes:
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
Nov 03 2006
parent Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
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)
Nov 03 2006
prev sibling parent Tomas Lindquist Olsen <tomas famolsen.dk> writes:
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!
Nov 03 2006