www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.signals regressions

reply David <d dav1d.de> writes:
This code currently fails with a RangeError (used to work in 2.062)

// http://dpaste.dzfl.pl/332a71ec
import std.stdio;
import std.signals;


struct X {
	mixin Signal!();
}

class O {
	void m() {}
}

void main() {
	O o = new O();
	X[string] aa;
	aa["o"] = X.init;
	aa["o"].connect(&o.m);
	

	/*{ // 20
		X x = aa["o"];
	}*/		
}


If you take the uncomment the "block" at line 20 you end up with a
segmentation fault, also worked in 2.062. Both times the problem is in
the __dtor (segfault happens in the call to "_d_toObject(stor.ptr)".

Changing "struct" to "class" and "X.init" to "new X()" it seems to work
as it should.
Is this worth a bugreport or was the old behaviour never intended?
This bug (I consider it one) broke quite a few lines of code... If the
old behaviour was never intended, why wasn't it documentated then... oh
well I am drifting into another D rant here...
Jun 14 2013
next sibling parent reply Denis Shelomovskij <verylonglogin.reg gmail.com> writes:
14.06.2013 18:31, David пишет:

 This code currently fails with a RangeError (used to work in 2.062)

 // http://dpaste.dzfl.pl/332a71ec
 import std.stdio;
 import std.signals;


 struct X {
 	mixin Signal!();
 }

 class O {
 	void m() {}
 }

 void main() {
 	O o = new O();
 	X[string] aa;
 	aa["o"] = X.init;
 	aa["o"].connect(&o.m);
 	

 	/*{ // 20
 		X x = aa["o"];
 	}*/		
 }


 If you take the uncomment the "block" at line 20 you end up with a
 segmentation fault, also worked in 2.062. Both times the problem is in
 the __dtor (segfault happens in the call to "_d_toObject(stor.ptr)".

 Changing "struct" to "class" and "X.init" to "new X()" it seems to work
 as it should.
 Is this worth a bugreport or was the old behaviour never intended?
 This bug (I consider it one) broke quite a few lines of code... If the
 old behaviour was never intended, why wasn't it documentated then... oh
 well I am drifting into another D rant here...


http://dlang.org/phobos/std_signals.html#Signal
"Mixin to create a signal within a class object."

So your `X` must be a class.

Also don't use std.signals - it's an incorrect and dangerous mess (see 
bug tracker).

-- 
Денис В. Шеломовский
Denis V. Shelomovskij
Jun 14 2013
parent David <d dav1d.de> writes:
15.06.2013 14:19, David пишет:

 http://dlang.org/phobos/std_signals.html#Signal
 "Mixin to create a signal within a class object."

 So your `X` must be a class.


 Oh right I have totally overseen that, still used to work really well
 with 2.063


It can work only by accident iff the struct isn't moved in memory no
matter what compiler version you use.

 Also don't use std.signals - it's an incorrect and dangerous mess (see
 bug tracker).


 Provide me something better in phobos and so far it works pretty good
 (except the update breaking struct-signals)


Currently there is no working signals implementation in Phobos. The
worst thing about the current implementation is it will fail you program
accidentally in rare cases. Don't use it.

For issues, see:
http://d.puremagic.com/issues/show_bug.cgi?id=4150
http://d.puremagic.com/issues/show_bug.cgi?id=9606
http://d.puremagic.com/issues/show_bug.cgi?id=9603


Also post to NG instead of emailing directly to allow others to follow
the discussion.

Also if you _do_ need working signals I can help a with implementation.

-- 
Денис В. Шеломовский
Denis V. Shelomovskij

-------------------------------------------------------------------------------------

Regarding E-mailing, sorry about that, I hate the thunderbird answer
button....


 It can work only by accident iff the struct isn't moved in memory no >


matter what compiler version you use.

This worked for at least 3 compiler versions now

I am aware of all of these bugs, none are critical if you know them
4150: use only class methods as event handlers
9606: emitting signals from a different thread would segfault in my
application anayways (opengl code)
9603: basically same as 4150

Also wasn't there a std.signals2 somewhere...
Jun 15 2013
prev sibling next sibling parent reply David <d dav1d.de> writes:
I looked into the "new std.signals" today:

https://github.com/eskimor/phobos/blob/new_signal/std/signals.d

It is completly unusable, "mixin Signal!() x" is blocked by bug, it
doesn't compile due to a wrong type (relativly easy fix), then when
using -unittest it fails to compile, compiler also doesn't give any
hints what exactly fails.
Removing these unittests makes it at least compile, but the other
unittests fail (assert). Removing also these unittests (hoping it still
works), nope it doesn't signals aren't triggered at all.

Are there any plans on improving this situation with std.signals or the
new implementation, currently both are getting less useable every day...
(e.g. signal.disconnect(&my_handler) disconnects all, yay what a joy)


I am hijacking this thread in hope someone will read that ;)
Hopefully also someone of the "new std.signals" developers.
Jun 16 2013
parent reply Robert <jfanatiker gmx.at> writes:
On Sun, 2013-06-16 at 15:25 +0200, David wrote:

 I looked into the "new std.signals" today:


Thanks for having a look :-)


 
 https://github.com/eskimor/phobos/blob/new_signal/std/signals.d
 
 It is completly unusable, "mixin Signal!() x" is blocked by bug, it
 doesn't compile due to a wrong type (relativly easy fix), then when
 using -unittest it fails to compile, compiler also doesn't give any
 hints what exactly fails.


Current master is an experimental CAS based implementation- untested and
very likely to get completely reworked internally.


 Removing these unittests makes it at least compile, but the other
 unittests fail (assert). Removing also these unittests (hoping it still
 works), nope it doesn't signals aren't triggered at all.
 
 Are there any plans on improving this situation with std.signals or the
 new implementation, currently both are getting less useable every day...
 (e.g. signal.disconnect(&my_handler) disconnects all, yay what a joy)


There are plans absolutely, to be concrete: I plan to make it ready in
July. There is one blocker currently:
http://d.puremagic.com/issues/show_bug.cgi?id=8441

but there is already an aging pull request which fixes it:

https://github.com/D-Programming-Language/dmd/pull/1660 (4 months old
already)

I hope it will be merged soon, then there is hopefully nothing that
hinders my plans. The very least it should become ready during summer.


 
 
 I am hijacking this thread in hope someone will read that ;)
 Hopefully also someone of the "new std.signals" developers.


I almost missed it ;-)

Sorry about your bad experiences with the current version.

Best regards,

Robert
Jun 17 2013
parent reply David <d dav1d.de> writes:
 https://github.com/eskimor/phobos/blob/new_signal/std/signals.d

 It is completly unusable, "mixin Signal!() x" is blocked by bug, it
 doesn't compile due to a wrong type (relativly easy fix), then when
 using -unittest it fails to compile, compiler also doesn't give any
 hints what exactly fails.


 
 Current master is an experimental CAS based implementation- untested and
 very likely to get completely reworked internally.


Good to know, but that is already a few monce old, or? I remember seeing
CAS in the code though


 Removing these unittests makes it at least compile, but the other
 unittests fail (assert). Removing also these unittests (hoping it still
 works), nope it doesn't signals aren't triggered at all.

 Are there any plans on improving this situation with std.signals or the
 new implementation, currently both are getting less useable every day...
 (e.g. signal.disconnect(&my_handler) disconnects all, yay what a joy)


 
 There are plans absolutely, to be concrete: I plan to make it ready in
 July. There is one blocker currently:
 http://d.puremagic.com/issues/show_bug.cgi?id=8441
 
 but there is already an aging pull request which fixes it:
 
 https://github.com/D-Programming-Language/dmd/pull/1660 (4 months old
 already)
 
 I hope it will be merged soon, then there is hopefully nothing that
 hinders my plans. The very least it should become ready during summer.


This blocker wasn't a problem, I used the FullSignal, I can live with
that, I don't like to have a private emit method anyways (in my opinion,
private is always wrong)


 I am hijacking this thread in hope someone will read that ;)
 Hopefully also someone of the "new std.signals" developers.


 
 I almost missed it ;-)
 
 Sorry about your bad experiences with the current version.


Hehe wouldn't say it is your fault, obviously it worked at some point (I
guess)... I blame DMD, Ranges and the GC of course ;)

- David
Jun 17 2013
parent reply Robert <jfanatiker gmx.at> writes:
 Current master is an experimental CAS based implementation- untested and
 very likely to get completely reworked internally.


 
 Good to know, but that is already a few monce old, or? I remember seeing
 CAS in the code though


It is yes. It is blocked and I got busy with other stuff, so I did put
it on hold. Current master never worked, commits prior to (and
including):

04c951e34623e9365a3874c89f43eb997a7b376c

should work (if you drop the mixin part). The Heisenbug is still present
though. (That's the reason for the CAS stuff)



 This blocker wasn't a problem, I used the FullSignal, I can live with
 that, I don't like to have a private emit method anyways (in my opinion,
 private is always wrong)


Then you might want to try out an older version, see above.


 Hehe wouldn't say it is your fault, obviously it worked at some point (I
 guess)... I blame DMD, Ranges and the GC of course ;)


Well master never worked (it is work in progress), but older versions
did and should still.

Best regards,

Robert
Jun 17 2013
parent David <d dav1d.de> writes:
Am 17.06.2013 22:23, schrieb Robert:

 Current master is an experimental CAS based implementation- untested and
 very likely to get completely reworked internally.


 Good to know, but that is already a few monce old, or? I remember seeing
 CAS in the code though


 It is yes. It is blocked and I got busy with other stuff, so I did put
 it on hold. Current master never worked, commits prior to (and
 including):
 
 04c951e34623e9365a3874c89f43eb997a7b376c
 
 should work (if you drop the mixin part). The Heisenbug is still present
 though. (That's the reason for the CAS stuff)


Thanks, might try again, because for what I wanna do now, I really need
strongConnect, so far it wasn't an issue but it's getting one...
Jun 17 2013
prev sibling parent reply Robert <jfanatiker gmx.at> writes:
I just finished a new implementation, replacing the template mixin with
a string mixin. I also changed the name from signals2 to signal. You can
find it here:

https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d

All unittests pass, you don't need any patched compiler. I still have to
add some more checks and do some polishing, I will also put it in the
dub registry. But you seem to have an urgent need, so feel free to try
it out immediately - Be my pre-alpha Tester :-)

Best regards,

Robert
Jul 12 2013
parent reply David <d dav1d.de> writes:
Am 12.07.2013 23:47, schrieb Robert:

 I just finished a new implementation, replacing the template mixin with
 a string mixin. I also changed the name from signals2 to signal. You can
 find it here:
 
 https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d
 
 All unittests pass, you don't need any patched compiler. I still have to
 add some more checks and do some polishing, I will also put it in the
 dub registry. But you seem to have an urgent need, so feel free to try
 it out immediately - Be my pre-alpha Tester :-)
 
 Best regards,
 
 Robert
 
 


Bad timing, just got "our"[1] own implementation.
If I have time, I'll play around with it! Thanks for the great work,
maybe we can get something working into phobos...


[1]https://github.com/AndrejMitrovic/new_signals
Jul 12 2013
next sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 7/13/13, David <d dav1d.de> wrote:

 Bad timing, just got "our"[1] own implementation.
 If I have time, I'll play around with it! Thanks for the great work,
 maybe we can get something working into phobos...


 [1]https://github.com/AndrejMitrovic/new_signals


Disclaimer: This is the work of Johannes Pfau which I've just hacked
and butchered a little bit (and also updated for 2.064) because we
needed std.signals that doesn't crash that often.
Jul 12 2013
parent reply "Robert" <jfanatiker gmx.at> writes:
On Saturday, 13 July 2013 at 01:24:11 UTC, Andrej Mitrovic wrote:

 On 7/13/13, David <d dav1d.de> wrote:

 Bad timing, just got "our"[1] own implementation.
 If I have time, I'll play around with it! Thanks for the great 
 work,
 maybe we can get something working into phobos...


 [1]https://github.com/AndrejMitrovic/new_signals


 Disclaimer: This is the work of Johannes Pfau which I've just 
 hacked
 and butchered a little bit (and also updated for 2.064) because 
 we
 needed std.signals that doesn't crash that often.


Wow, I wasn't aware of either this implementation nor this 
thread. Thanks for sharing.
Jul 13 2013
parent reply "Robert" <jfanatiker gmx.at> writes:
 [1]https://github.com/AndrejMitrovic/new_signals







Hmm, ok this implementation does not implement weak ref 
semantics, but it does implement some fancy features like 
enabling/disabling emission, adding slots at a specified position 
and stuff. At least for enabling/disabling I decided not to 
include it for now, to keep the implementation simple and I also 
think it is not really needed. But in general what features of 
your new_signals implentation do you really need/like that are 
not present in my implementation? Ideally with use cases so I can 
get an idea? Thanks for the feedback!

Best regards,

Robert
Jul 13 2013
parent reply David <d dav1d.de> writes:
Am 13.07.2013 10:14, schrieb Robert:

 

 [1]https://github.com/AndrejMitrovic/new_signals






 
 
 Hmm, ok this implementation does not implement weak ref semantics, but
 it does implement some fancy features like enabling/disabling emission,
 adding slots at a specified position and stuff. At least for
 enabling/disabling I decided not to include it for now, to keep the
 implementation simple and I also think it is not really needed. But in
 general what features of your new_signals implentation do you really
 need/like that are not present in my implementation? Ideally with use
 cases so I can get an idea? Thanks for the feedback!
 
 Best regards,
 
 Robert


What I like? It works and it was a opt-in for my old code (s/mixin
Signal/Signal/), but you need to probably break the API-compatability
with the weak ref thing.
I don't really need the "stop propagation", "insert before/after", but I
can see where it can come in handy.

So far, I was really really happy when Andrej came up with this code, I
really really needed something else than the old std.signals and this
does exactly what I needed.
Jul 13 2013
parent reply Johannes Pfau <nospam example.com> writes:
Am Sat, 13 Jul 2013 15:47:35 +0200
schrieb David <d dav1d.de>:


 Am 13.07.2013 10:14, schrieb Robert:

 

 [1]https://github.com/AndrejMitrovic/new_signals






 
 
 Hmm, ok this implementation does not implement weak ref semantics,
 but it does implement some fancy features like enabling/disabling
 emission, adding slots at a specified position and stuff. At least
 for enabling/disabling I decided not to include it for now, to keep
 the implementation simple and I also think it is not really needed.
 But in general what features of your new_signals implentation do
 you really need/like that are not present in my implementation?
 Ideally with use cases so I can get an idea? Thanks for the
 feedback!
 
 Best regards,
 
 Robert


 
 What I like? It works and it was a opt-in for my old code (s/mixin
 Signal/Signal/), but you need to probably break the API-compatability
 with the weak ref thing.
 I don't really need the "stop propagation", "insert before/after",
 but I can see where it can come in handy.
 
 So far, I was really really happy when Andrej came up with this code,
 I really really needed something else than the old std.signals and
 this does exactly what I needed.


When I initially wrote that code I wanted it to support everything that
usual C/C++ signal implementations support. I looked at what features
glib offered and implemented most of them.

I was never sure whether a list as the internal data structure was the
right choice though. I wanted to have a look at boost's signal
implementation and especially do some more performance testing. Then I
somehow forgot about it and it never seemed like there was a big
demand for signals so I never finished that work.

Anyway feel free to take anything you need from that code. I
unfortunately don't have much time right now to help integrate those
implementations but merging them would probably be a good idea.

But one comment though: Do you really need string mixins? I think
"Signal!int mysignal;" is a much nicer syntax in D than using mixins /
string mixins. And I think it's also quite important that a signal
implementation works with all of D's callable types - especially
functions and delegates but opCall is nice as well.
Jul 13 2013
parent reply "Robert" <jfanatiker gmx.at> writes:
 But one comment though: Do you really need string mixins? I 
 think
 "Signal!int mysignal;" is a much nicer syntax in D than using


I agree and you don't need the string mixin, it is just for 
convenience. The signal itself is a struct. But what was 
important to me was that one can easily restrict emitting the 
signal to the containing class. This is where you need some kind 
of mixin to avoid boilerplate. But if you don't need that or want 
to write the boilerplate yourself you can just use the FullSignal 
struct directly.

I wanted to use template mixins because of the little less 
cluttered syntax, but as it turns out they are quite buggy, so I 
changed it to a string mixin with the added flexibility that you 
can select the protection of FullSignal, with private being the 
default. The syntax does not turn out to be that bad:

   mixin(signal!(string, int)("valueChanged"));

or if you want for example protected instead of private:

   mixin(signal!(string, int)("valueChanged", "protected"));

If you don't want to use the mixin and don't care that everyone 
can emit the signal:
   FullSignal!(string, int) valueChanged;


You see the mixin is just a feature, if you don't like it - don't 
use it :-)


 mixins /
 string mixins. And I think it's also quite important that a 
 signal
 implementation works with all of D's callable types - especially
 functions and delegates but opCall is nice as well.


It does work with all callable types: Delegates/functions and by 
means of delegates with any callable type.

Best regards,
Robert
Jul 13 2013
parent Johannes Pfau <nospam example.com> writes:
Am Sat, 13 Jul 2013 22:37:45 +0200
schrieb "Robert" <jfanatiker gmx.at>:


 

 But one comment though: Do you really need string mixins? I 
 think
 "Signal!int mysignal;" is a much nicer syntax in D than using


 
 I agree and you don't need the string mixin, it is just for 
 convenience. The signal itself is a struct. But what was 
 important to me was that one can easily restrict emitting the 
 signal to the containing class.


Now that you mention it I also remember that issue. That's a good
reason to use the string/template mixin, although for me clearer syntax
had higher priority.
Jul 14 2013
prev sibling parent reply "Damian" <damianday hotmail.co.uk> writes:
On Friday, 12 July 2013 at 22:35:06 UTC, David wrote:

 Am 12.07.2013 23:47, schrieb Robert:

 I just finished a new implementation, replacing the template 
 mixin with
 a string mixin. I also changed the name from signals2 to 
 signal. You can
 find it here:
 
 https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d
 
 All unittests pass, you don't need any patched compiler. I 
 still have to
 add some more checks and do some polishing, I will also put it 
 in the
 dub registry. But you seem to have an urgent need, so feel 
 free to try
 it out immediately - Be my pre-alpha Tester :-)
 
 Best regards,
 
 Robert
 
 


 Bad timing, just got "our"[1] own implementation.
 If I have time, I'll play around with it! Thanks for the great 
 work,
 maybe we can get something working into phobos...


 [1]https://github.com/AndrejMitrovic/new_signals


Having tried this I can't get ref parameters to work as function 
arguments, are they supported?

Example:

struct KeyEvent {}

public Signal!(ref KeyEvent) onKeyDown;

connect, emit ...

public void keyDownHandler(ref KeyEvent keyEvent) { }

Works fine without ref.
Jul 13 2013
parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 7/14/13, Damian <damianday hotmail.co.uk> wrote:

 Having tried this I can't get ref parameters to work as function
 arguments, are they supported?


This is a language issue, you can't pass 'ref' as a template parameter.
Jul 13 2013