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.

with 2.063

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

(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 >

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
next sibling 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 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 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 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 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
prev sibling next 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
parent 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
prev sibling next sibling parent 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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent 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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling 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
prev sibling 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