Bug 9347 – new std.signals2 implementation

Status
RESOLVED
Resolution
INVALID
Severity
enhancement
Priority
P2
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2013-01-18T03:49:00Z
Last change time
2016-01-18T06:22:59Z
Assigned to
nobody
Creator
jfanatiker
Blocks
2447

Comments

Comment #0 by jfanatiker — 2013-01-18T03:49:56Z
The current std.signals implementation has various short comings. I already described in the mailing list: http://forum.dlang.org/thread/[email protected] The features of the new implementation: http://forum.dlang.org/thread/[email protected] Code can be found at: https://github.com/eskimor/phobos/blob/new_signal/std/signals.d It works (except for the template mixin part), see issue: 8441. A little cleanup, a review and rename to std.signals2 would still be needed.
Comment #1 by verylonglogin.reg — 2013-02-27T02:49:19Z
(In reply to comment #0) > The current std.signals implementation has various short comings. For closure related shortcomings see Issue 9603. I also would like to have a compiler solution of it instead of a library one (see Issue 9603 Comment 1).
Comment #2 by jfanatiker — 2013-02-28T05:43:16Z
(In reply to comment #1) > (In reply to comment #0) > > The current std.signals implementation has various short comings. > > For closure related shortcomings see Issue 9603. I also would like to have a > compiler solution of it instead of a library one (see Issue 9603 Comment 1). The new implementation supports wrapping delegates, this is fact was a very important feature for me.
Comment #3 by verylonglogin.reg — 2013-02-28T07:11:02Z
(In reply to comment #2) > The new implementation supports wrapping delegates, this is fact was a very > important feature for me. But it doesn't do it correctly as it can't because of absence of stuff for controlling/watching delegate lifetime in D. See bug 9603 comment 2 for a test case. Also new implementation doesn't handle threading issue 9606.
Comment #4 by jfanatiker — 2013-02-28T07:48:50Z
(In reply to comment #3) > (In reply to comment #2) > > The new implementation supports wrapping delegates, this is fact was a very > > important feature for me. > > But it doesn't do it correctly as it can't because of absence of stuff for > controlling/watching delegate lifetime in D. See bug 9603 comment 2 for a test > case. It does, because it holds a strong ref to the delegate context, but a weak ref to the target object. The following: obj.actionDone.connect(i => watch(i)); would become: obj.actionDone.connect!Observer(this, (o, i) => o.watch(i)); with the new implementation. Might not be as pretty, but is the only way I could think of to make wrapper delegates work properly. This also enables the signal to provide the strongConnect() methods which allows connecting to non object targets, but with strong ref semantics. The signal needs to have a strong ref to the delegate's context, but also needs to have a weak ref to the target object. In addition the delegate's context can not contain a reference to the target object, because this would simply destroy the weak ref semantics. Decoupling the two and passing the object to the delegate via a parameter, seems to be the only way to solve this issue. and at least in my opinion not an entirely bad one. > > Also new implementation doesn't handle threading issue 9606. Thanks for pointing this out, I will fix it.
Comment #5 by verylonglogin.reg — 2013-03-16T05:03:59Z
(In reply to comment #4) > > Also new implementation doesn't handle threading issue 9606. > > Thanks for pointing this out, I will fix it. Probably weak references (issue 4151) have to be implemented first.
Comment #6 by jfanatiker — 2013-03-16T05:34:52Z
(In reply to comment #5) > (In reply to comment #4) > > > Also new implementation doesn't handle threading issue 9606. > > > > Thanks for pointing this out, I will fix it. > > Probably weak references (issue 4151) have to be implemented first. They would of course be nice and make the signal implementation trivial. But they are not strictly required, I already tackled the issue in the current master of my implementation. (Not yet tested, won't even compile, but the basic concept should work) Best regards, Robert
Comment #7 by jack — 2016-01-18T06:22:59Z
This sounds less like a enhancement request and more like an announcement of your solution to a problem. If you want this in Phobos, then make a PR for it. Marking this as invalid because this is not asking for an enhancement.