www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Review of std.signal

reply "Dicebot" <public dicebot.lv> writes:
Time to move forward with some more reviews :)

Our current victim is std.signal by Robert Klotzner which is 
supposed to deprecate and supersede existing (and broken) 
std.signals

--- Input ---

Code: 
https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d
Docs: 
https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html
Dub package: http://code.dlang.org/packages/phobosx

--- Review format ---

While proposal is expected to be somewhat solid and ready, it is 
a first formal review happening with all previous discussions 
scattered around the various threads (unless I have missed 
something). So intention right now is more to gather some 
destructive input but we may proceed straight to voting if 
reviews will be overwhelmingly positive.

--- Usual information for reviewers ---

Please take this seriously: "If you identify problems along the
way, please note if they are minor, serious, or showstoppers."
(http://wiki.dlang.org/Review/Process). This information later
will be used to determine if library is ready for voting.

If there are any frequent Phobos contributors / core developers
please pay extra attention to submission code style and fitting
into overall Phobos guidelines and structure.
Nov 07 2013
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-11-07 14:45, Dicebot wrote:
 Time to move forward with some more reviews :)

 Our current victim is std.signal by Robert Klotzner which is supposed to
 deprecate and supersede existing (and broken) std.signals

* If a template mixin, which uses the string mixin, is provided the syntax will look a bit nicer * Isn't it better to use an enum for the protection attributes? * Would it make sense to allow "export" as a protection attribute as well? -- /Jacob Carlborg
Nov 07 2013
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-11-08 11:22, Robert wrote:

 I agree that template mixin syntax is a bit nicer, but I ran into a few
 issues with them. In the end I settled with the string mixin, because it
 avoids those issues and also was more powerful (User can now choose the
 protection). How would your template mixin wrapper look like?

Just take the same arguments as the "signal" function, something like: template signal (string name, string protection, Args...) { mixin(signalImpl!(Args)(name, protection)); }
 Oh, quite the other way round, the public in the assert list is quite
 unnecessary. If you want to go public/export just use "none" as
 protection and set it yourself, like:
 public {
       mixin(signal!(string, int)("valueChanged", "none"));
 }

If that works, why do you have the option to specify the protection at all? BWT, shouldn't that assert be static? -- /Jacob Carlborg
Nov 08 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-11-09 23:50, Robert wrote:
1
 Nice. But you trade it for protection having a default value, making the
 syntax more verbose in the general case.

You can overload it: template signal (string name, Args...)
 So you can have a different protection for the full signal
 implementation and for the restricted part.

Ok, I see.
 Well the function is executed at compile time, but protection is still a
 "runtime" argument (so to speak) so I guess the answer is no.

Right. -- /Jacob Carlborg
Nov 10 2013
prev sibling parent "Tyro[17]" <ridimz yahoo.com> writes:
On 11/14/13, 8:40 AM, Robert wrote:
 * Isn't it better to use an enum for the protection attributes?

Having put some thought into this, while it seems to be, I think it isn't. Enums in D are scoped, so for the enum: enum Protection { protected_, private_, none }

Enums in D may be scoped. There is nothing that prevents you from declaring your enum thus: enum { protected_, private_, none } making the shorter version perfectly legal. mixin(signal!(string, int)("valueChanged", protected_)); Your concern about extra typing by always needing to specify the scope is thus unwarranted.
Nov 14 2013
prev sibling next sibling parent "Daniel Kozak" <kozzi11 gmail.com> writes:
On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Time to move forward with some more reviews :)

 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

code: private: RestrictedSignal!(Args) restricted_; And first what came to my mind was that I do not know restricted keyword. But than I find out there is no such keyword. So why is there a underscore at the end instead of at the beginning?
Nov 07 2013
prev sibling next sibling parent "Michael" <pr m1xa.com> writes:
On Thursday, 7 November 2013 at 21:23:36 UTC, Daniel Kozak wrote:
 On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Time to move forward with some more reviews :)

 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

code: private: RestrictedSignal!(Args) restricted_; And first what came to my mind was that I do not know restricted keyword. But than I find out there is no such keyword. So why is there a underscore at the end instead of at the beginning?

private member variable of typeof ( RestrictedSignal!Args )
Nov 07 2013
prev sibling next sibling parent "Daniel Kozak" <kozzi11 gmail.com> writes:
On Thursday, 7 November 2013 at 21:26:38 UTC, Michael wrote:
 On Thursday, 7 November 2013 at 21:23:36 UTC, Daniel Kozak 
 wrote:
 On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Time to move forward with some more reviews :)

 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

of code: private: RestrictedSignal!(Args) restricted_; And first what came to my mind was that I do not know restricted keyword. But than I find out there is no such keyword. So why is there a underscore at the end instead of at the beginning?

private member variable of typeof ( RestrictedSignal!Args )

but still what I understand from doc (http://dlang.org/dstyle.html) it should be called _restricted not restricted_
Nov 07 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:

code: private: RestrictedSignal!(Args) restricted_; And first what came to my mind was that I do not know restricted keyword. But than I find out there is no such keyword. So why is there a underscore at the end instead of at the beginning?

Thanks, was an oversight. Fixed in master.
Nov 08 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Thursday, 7 November 2013 at 20:48:58 UTC, Jacob Carlborg 
wrote:

 * If a template mixin, which uses the string mixin, is provided 
 the syntax will look a bit nicer

I agree that template mixin syntax is a bit nicer, but I ran into a few issues with them. In the end I settled with the string mixin, because it avoids those issues and also was more powerful (User can now choose the protection). How would your template mixin wrapper look like?
 * Isn't it better to use an enum for the protection attributes?

Not sure as the protection attributes are keywords. I'll think about it, do others agree that an enum would be better with private_, protected_, .... members ?
 * Would it make sense to allow "export" as a protection 
 attribute as well?

Oh, quite the other way round, the public in the assert list is quite unnecessary. If you want to go public/export just use "none" as protection and set it yourself, like: public { mixin(signal!(string, int)("valueChanged", "none")); } Thanks for the feedback.
Nov 08 2013
prev sibling next sibling parent "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Friday, 8 November 2013 at 10:22:53 UTC, Robert wrote:
 On Thursday, 7 November 2013 at 20:48:58 UTC, Jacob Carlborg 
 wrote:

 * If a template mixin, which uses the string mixin, is 
 provided the syntax will look a bit nicer

I agree that template mixin syntax is a bit nicer, but I ran into a few issues with them. In the end I settled with the string mixin, because it avoids those issues and also was more powerful (User can now choose the protection). How would your template mixin wrapper look like?

Why does this need to be a mixin at all? The only reason I see is that it introduces two declarations, but I don't see why two are needed. At any rate, a string mixin is not necessitated here at all, and really hurts code readability (though mostly on the implementation side). If you're having issues with templates, please tell us so we can help.
Nov 08 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 At any rate, a string mixin is not necessitated here at all, 
 and really hurts code readability (though mostly on the 
 implementation side). If you're having issues with templates, 
 please tell us so we can help.

You don't need it, that is true. You can use the Signal struct directly, it is only there for convenience. It restricts permission to emit the signal to the module, by wrapping it up into an accessor method. Best regards, Robert
Nov 08 2013
prev sibling next sibling parent "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Friday, 8 November 2013 at 14:27:39 UTC, Robert wrote:
 You don't need it, that is true. You can use the Signal struct 
 directly, it is only there for convenience. It restricts 
 permission to emit the signal to the module, by wrapping it up 
 into an accessor method.

 Best regards,
 Robert

I was referring to the issue of string mixin vs mixin template.
Nov 08 2013
prev sibling next sibling parent "Anonimous" <tr1 google.com> writes:
Is it reasonable to introduce new identifier _sig for each signal 
sig?
You could just make emit private static function,that takes 
signal by the first argument instead.
Nov 08 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 I was referring to the issue of string mixin vs mixin template.

My bad ;-) My first implementation was using just a template mixin, but I hit a few bugs (At least 2, one should be fixed already the other one I can't remember, but had something to do with derived classes), so I tried the string mixin approach and I have to admit, that I like it more now, regardless of template mixin bugs. The added flexibility of choosing the protection is a big win as it is not that unlikely that someone wants for example derived classes being able to emit the signal. Another minor drawback of the template mixin, is that I have to wrap all struct members into mixin functions*) and the actual struct would need to be renamed into something less straight forward too. Best regards, Robert *) Without wrapping, using the signal would be more ugly, something like: class Test { mixin Signal!int a; mixin(signal!int("b")); void doSomething() { // something a.signal.emit(4); // vs b.emit(4); } }
Nov 08 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Friday, 8 November 2013 at 16:13:04 UTC, Anonimous wrote:
 Is it reasonable to introduce new identifier _sig for each 
 signal sig?

I think so, yes. It is more or less the code one would write by hand if there were no mixin.
 You could just make emit private static function,that takes 
 signal by the first argument instead.

I don't understand that. :-(
Nov 08 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 Just take the same arguments as the "signal" function, 
 something like:

 template signal (string name, string protection, Args...)
 {
     mixin(signalImpl!(Args)(name, protection));
 }

Nice. But you trade it for protection having a default value, making the syntax more verbose in the general case.
 If that works, why do you have the option to specify the 
 protection at all?

So you can have a different protection for the full signal implementation and for the restricted part.
 BWT, shouldn't that assert be static?

Well the function is executed at compile time, but protection is still a "runtime" argument (so to speak) so I guess the answer is no. Best regards, Robert
Nov 09 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Sunday, 10 November 2013 at 10:37:41 UTC, Jacob Carlborg wrote:
 On 2013-11-09 23:50, Robert wrote:
 1
 Nice. But you trade it for protection having a default value, 
 making the
 syntax more verbose in the general case.

You can overload it: template signal (string name, Args...)

Smartass! ;-) Of course this would work, not bad! I am not sure whether this additional wrapper it is worth its salt, but it is definitely a nice alternative. I will absolutely think about it. Thanks!
Nov 10 2013
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Sunday, 10 November 2013 at 11:32:45 UTC, Robert wrote:
 On Sunday, 10 November 2013 at 10:37:41 UTC, Jacob Carlborg 
 wrote:
 On 2013-11-09 23:50, Robert wrote:
 1
 Nice. But you trade it for protection having a default value, 
 making the
 syntax more verbose in the general case.

You can overload it: template signal (string name, Args...)

Smartass! ;-) Of course this would work, not bad! I am not sure whether this additional wrapper it is worth its salt, but it is definitely a nice alternative. I will absolutely think about it. Thanks!

You can use this syntax: template signal (ProtectionType protection = ProtectionType.private_, string name, Args...) { mixin(signalImpl!(Args)(name, protection)); } enum ProtectionType { none, private_, //... } It combines both Jacob's suggestions. Also, it looks like that you don't need `string mixin` at all. You can use `static if`.
Nov 10 2013
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
Upping the thread as it did not receive much review attention is 
is slowly getting fading away.

Robert, for someone who has only vague knowledge of signal stuff 
(me), can you explain why exactly this design was necessary? 
(mixing in global variable and wrapper function which returns its 
restricted version) Is the desire to separate protection 
attributes the only motivation or there are some other concerns?
Nov 12 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 Robert, for someone who has only vague knowledge of signal 
 stuff (me), can you explain why exactly this design was 
 necessary? (mixing in global variable and wrapper function 
 which returns its restricted version) Is the desire to separate 
 protection attributes the only motivation or there are some 
 other concerns?

No this is the only reason. Usually it is the common case that you only want the "owner" of the signal to emit it. Without the mixin, this would require the following boilerplate for every single signal: ref RestrictedSignal!(SomeTemplate!int) mysig() { return _mysig;} private Signal!(SomeTemplate!int) _mysig; which is a bit tedious. I am currently considering the suggestions of Jacob Carlborg: - using an enum for the protections - provide an additional template mixin wrapper for another minor reduction of boilerplate. Signal slot frameworks in C++ usually either only allow the owner to emit the signal (Qt) or you are required to write some wrapper functions for restrictions on the public API (e.g. boost signal). Best regards, Robert
Nov 12 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 * If a template mixin, which uses the string mixin, is provided 
 the syntax will look a bit nicer

It does, as it saves two to four brackets for multi arg signals, I nevertheless decided against it for the following reasons: 1. Despite being a little more verbose, I like its style better. Maybe because the name of the signal is separate from its parameter types, providing a bit more structure. 2. Naming problem. The template would have to start with a capital letter according to the D style rules, thus it would clash with the struct Signal. I could name it Sig or something or rename the struct. A stupid reason, but it troubles me a bit. 3. It is just another level of indirection making it arguably harder to understand what is going on: template mixin -> string mixin -> struct.
 * Isn't it better to use an enum for the protection attributes?

Having put some thought into this, while it seems to be, I think it isn't. Enums in D are scoped, so for the enum: enum Protection { protected_, private_, none } The syntax gets more verbose instead of less: mixin(signal!(string, int)("valueChanged", Protection.protected_)); instead of: mixin(signal!(string, int)("valueChanged", "protected")); Also you can use the keyword as is and as every programmer is used to. Pro enum would be tool support (auto completion) but I suppose people would almost always use the default anyway and if they don't, typing "protected" by hand is not really a challenge. But thanks for the suggestions, I had not even thought about the wrapping template mixin before. If you or someone else are really unhappy with those decisions, feel free to argue against them ;-) . Best regards, Robert
Nov 14 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 Enums in D may be scoped. There is nothing that prevents you 
 from declaring your enum thus:

 enum { protected_, private_, none }

 making the shorter version perfectly legal.

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

 Your concern about extra typing by always needing to specify 
 the scope is thus unwarranted.

This would work, but would make the protection parameter an int. Not really any more beautiful than a string. The advantage of the enum for me was, that it is clear from the signature what are valid protections, this advantage gets lost with an anonymous enum. The only remaining advantage would be tool support for completion, but I don't think it is worth it.
Nov 14 2013
prev sibling next sibling parent "John Colvin" <john.loughran.colvin gmail.com> writes:
On Thursday, 14 November 2013 at 15:40:49 UTC, Robert wrote:
 Enums in D may be scoped. There is nothing that prevents you 
 from declaring your enum thus:

 enum { protected_, private_, none }

 making the shorter version perfectly legal.

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

 Your concern about extra typing by always needing to specify 
 the scope is thus unwarranted.

This would work, but would make the protection parameter an int. Not really any more beautiful than a string. The advantage of the enum for me was, that it is clear from the signature what are valid protections, this advantage gets lost with an anonymous enum. The only remaining advantage would be tool support for completion, but I don't think it is worth it.

enum Protection { protected_, private_, none } alias protected_ = Protection.protected_; alias private_ = Protection.private_; alias none = Protection.none; Best of both worlds?
Nov 14 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 enum Protection { protected_, private_, none }
 alias protected_ = Protection.protected_;
 alias private_ = Protection.private_;
 alias none = Protection.none;

 Best of both worlds?

I thought of this too and it does seem to be a viable solution. I personally really don't mind the string parameter, after all it is a string mixin and it has a default value. But I have to admit, seeing it right in front of me, you got me there, I might like it.
Nov 14 2013
prev sibling next sibling parent "John Colvin" <john.loughran.colvin gmail.com> writes:
On Thursday, 14 November 2013 at 21:13:24 UTC, Robert wrote:
 enum Protection { protected_, private_, none }
 alias protected_ = Protection.protected_;
 alias private_ = Protection.private_;
 alias none = Protection.none;

 Best of both worlds?

I thought of this too and it does seem to be a viable solution. I personally really don't mind the string parameter, after all it is a string mixin and it has a default value. But I have to admit, seeing it right in front of me, you got me there, I might like it.

The only argument against it is namespace pollution.
Nov 14 2013
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Thursday, 14 November 2013 at 21:48:16 UTC, John Colvin wrote:
 On Thursday, 14 November 2013 at 21:13:24 UTC, Robert wrote:
 enum Protection { protected_, private_, none }
 alias protected_ = Protection.protected_;
 alias private_ = Protection.private_;
 alias none = Protection.none;

 Best of both worlds?

I thought of this too and it does seem to be a viable solution. I personally really don't mind the string parameter, after all it is a string mixin and it has a default value. But I have to admit, seeing it right in front of me, you got me there, I might like it.

The only argument against it is namespace pollution.

There is no such problem in D.
Nov 14 2013
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

Robert, it will be great to see some more documentation for `std.signal`. For example, you can: - add more usage examples for each public function - add tread-safe example for multi-tread application - add better signal description and add more external links (for examle, to the wikipedia - http://en.wikipedia.org/wiki/Signals_and_slots). Look at the old std.signals - http://dlang.org/phobos/std_signals.html
Nov 15 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
I changed the string mixin now to take an enum instead of the 
string for specifying the protection. You convinced me, it is 
just nicer.

  https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html
  
https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d

I will also look into improving documentation a bit soon.

Best regards,
Robert

 Robert, it will be great to see some more documentation for 
 `std.signal`.
 For example, you can:
  - add more usage examples for each public function
  - add tread-safe example for multi-tread application
  - add better signal description and add more external links 
 (for examle, to the wikipedia - 
 http://en.wikipedia.org/wiki/Signals_and_slots). Look at the 
 old std.signals - http://dlang.org/phobos/std_signals.html

Nov 18 2013
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Tuesday, 19 November 2013 at 07:10:32 UTC, Robert wrote:
 I changed the string mixin now to take an enum instead of the 
 string for specifying the protection. You convinced me, it is 
 just nicer.

  https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html
  
 https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d

 I will also look into improving documentation a bit soon.

 Best regards,
 Robert

 Robert, it will be great to see some more documentation for 
 `std.signal`.
 For example, you can:
 - add more usage examples for each public function
 - add tread-safe example for multi-tread application
 - add better signal description and add more external links 
 (for examle, to the wikipedia - 
 http://en.wikipedia.org/wiki/Signals_and_slots). Look at the 
 old std.signals - http://dlang.org/phobos/std_signals.html


Thanks! Looks there are no real objections against the proposal but I am rather concerned about general lack of activity during review. As a compromise, what about putting it in "candidate" category (http://code.dlang.org/?sort=updated&category=library.std_aspirant) and making 1-2 month pause before proceeding to actual voting? That will also fix minor issue of me being a bit busy and not paying proper attention :)
Nov 22 2013
prev sibling next sibling parent "robert" <jfanatiker gmx.at> writes:
 Thanks!

 Looks there are no real objections against the proposal but I 
 am rather concerned about general lack of activity during 
 review. As a compromise, what about putting it in "candidate" 
 category 
 (http://code.dlang.org/?sort=updated&category=library.std_aspirant) 
 and making 1-2 month pause before proceeding to actual voting? 
 That will also fix minor issue of me being a bit busy and not 
 paying proper attention :)

Well it was already on the registry for a few months, I don't really see what another two months would gain. Considering the current state of std.signals and how nothing has happened to it in years, it does not seem that there is a huge amount of people interested in signals for D, I also don't see this to change within the next two months. I personally would like to continue going on to voting and get it over with, so I can clear this item from my todo list ;-) That being said, I of course understand if you are busy right now, a problem well known, so in the end it is your call. Thanks again for doing this in the first place! Best regards, Robert
Nov 22 2013
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Friday, 22 November 2013 at 16:07:56 UTC, robert wrote:
 Well it was already on the registry for a few months, I don't 
 really see what another two months would gain.

With a confusing name and not in expected category ;) I think renaming it to be visible by "signal" search query in package list + making D.announcement with "please please try to use it!" should make things a bit better. I personally can get back to this stuff only by the middle-end of December so there is no much else that can be done right now :(
Nov 22 2013
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
Robert, please add `RestrictedSignal(Args...).strongConnect(void 
function(Args) dg)` method or at least add link to the 
`std.functional.toDelegate` function.
In current situation your example doesn't work (easy to fix via 
toDelegate):

Error: function phobosx.signal.RestrictedSignal!(string, 
int).RestrictedSignal.strongConnect (void delegate(string, int) 
dg) is not callable using argument types (void function(string, 
int))
Nov 22 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Friday, 22 November 2013 at 18:20:37 UTC, ilya-stromberg wrote:
 Robert, please add 
 `RestrictedSignal(Args...).strongConnect(void function(Args) 
 dg)` method or at least add link to the 
 `std.functional.toDelegate` function.
 In current situation your example doesn't work (easy to fix via 
 toDelegate):

 Error: function phobosx.signal.RestrictedSignal!(string, 
 int).RestrictedSignal.strongConnect (void delegate(string, int) 
 dg) is not callable using argument types (void function(string, 
 int))

That's weird. It works in the unit test, I'll look into it. Thanks!
Nov 22 2013
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Friday, 22 November 2013 at 18:33:31 UTC, Robert wrote:
 That's weird. It works in the unit test, I'll look into it. 
 Thanks!

No, it isn't. The function `void watch(string msg, int i)` from unit tests is declarated into the unittest block. So, it's delegate because it contains pointer to the unittest scope. Try to change function to the `static void watch(string msg, int i)` and you'll see the same error.
Nov 22 2013
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
Sorry, I completely missed this message. Thanks for the hint, I 
fixed the documentation.

Best regards,

Robert

 No, it isn't.
 The function `void watch(string msg, int i)` from unit tests is 
 declarated into the unittest block. So, it's delegate because 
 it contains pointer to the unittest scope.
 Try to change function to the `static void watch(string msg, 
 int i)` and you'll see the same error.

Jan 04 2014
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Friday, 15 November 2013 at 11:25:30 UTC, ilya-stromberg wrote:
 On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

Robert, it will be great to see some more documentation for `std.signal`. For example, you can: - add more usage examples for each public function

I think this is overkill for this simple API. I would basically have to repeat the example given at the top for every single function, not really useful.
  - add tread-safe example for multi-tread application

std.signal is in no way special regarding multi threading, it is just the same as with every other module in phobos.
  - add better signal description and add more external links 
 (for examle, to the wikipedia - 
 http://en.wikipedia.org/wiki/Signals_and_slots). Look at the 
 old std.signals - http://dlang.org/phobos/std_signals.html

Done :-)
Jan 04 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Saturday, 4 January 2014 at 18:01:17 UTC, Robert wrote:
 On Friday, 15 November 2013 at 11:25:30 UTC, ilya-stromberg 
 wrote:
 On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

Robert, it will be great to see some more documentation for `std.signal`. For example, you can: - add more usage examples for each public function

I think this is overkill for this simple API. I would basically have to repeat the example given at the top for every single function, not really useful.

OK.
 - add tread-safe example for multi-tread application

std.signal is in no way special regarding multi threading, it is just the same as with every other module in phobos.

Not exactly. Almost all Phobos functions can be used for thread-local variables. Theoretically, `std.signal` can be used for multi-thread applications. So, it will be great to know what should I do for it. For example, must I use mutex to protect `Signal` method calls? So, please add example or at least add documentation how to do it.
 - add better signal description and add more external links 
 (for examle, to the wikipedia - 
 http://en.wikipedia.org/wiki/Signals_and_slots). Look at the 
 old std.signals - http://dlang.org/phobos/std_signals.html

Done :-)

Thanks.
Jan 04 2014
prev sibling next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Time to move forward with some more reviews :)

Hi, Apologies, I missed this review thread. So, some questions / observations: IMO, the effort to disallow third parties to "emit" signals on your object by far exceeds its worth. For this purpose, you have needed: - string mixins - ...which declare, by convention, a field with a name not explicitly given by the user - ...with a workaround for when they don't work - an enumeration type - two different signal struct types - a boatload of documentation - a nontrivial cognitive effort on the user to grok all of the above This little feature steals too much focus, and IMO is not worth it. As a compromise, have you considered template mixins? mixin template M() { public void pub() {} private void pri() {} } struct S { mixin M m; } Trying to access S.m.pri() from another module fails as expected. The downside is that the ".m" is not required, so the mixin members will "spill over" the parent object's field namespace.
 This implementation supersedes the former std.signals

I don't think comparisons of older versions of a module belong in the module's documentation.
 a.valueChanged.connect!"watch"(o);        // o.watch is the slot

This is really ugly, but I don't see any way to improve it either. Maybe if the type of &classInstance.method was actually a special "pointer-to-class-method" type that implicitly down-casted to a delegate, it would be possible to safely use &o.watch instead...
 dg must not be null. (Its context may.)

I don't think the context will ever be null (unless you take the address of a null class/struct pointer's method). (toDelegate uses the context pointer to store the function pointer, and the function pointer to store a template instance according to the function's arguments.)
 It implements the emit function for all other functionality it 
 has this aliased to RestrictedSignal.

Missing some punctuation, I think.
  The idea is to instantiate a Signal privately and provide a 
 public accessor method for accessing the contained 
 RestrictedSignal.

I had a hard time understanding this line the first time. Please reword without using "The idea is", because it's not clear what "the idea" is for - usage? Rationalization of implementation method?
 Use this overload if you either really, really want strong ref 
 semantics for some reason

Was the emphasis really necessary? It makes complete sense to build an asynchronous-event-model application using 100% only "strong connections" (delegates or delegate arrays). I don't remember ever NEEDING weak-referenced callbacks.
Jan 06 2014
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2014-01-06 11:13, Vladimir Panteleev wrote:

 Apologies, I missed this review thread. So, some questions / observations:

 IMO, the effort to disallow third parties to "emit" signals on your
 object by far exceeds its worth.

 For this purpose, you have needed:
 - string mixins
    - ...which declare, by convention, a field with a name not explicitly
 given by the user
    - ...with a workaround for when they don't work
 - an enumeration type
 - two different signal struct types
 - a boatload of documentation
 - a nontrivial cognitive effort on the user to grok all of the above

 This little feature steals too much focus, and IMO is not worth it.

I agree that it's not so nice looking. But I feel that it could be quite important to restrict emitting to the module the signal is declared in.
 a.valueChanged.connect!"watch"(o);        // o.watch is the slot

This is really ugly, but I don't see any way to improve it either. Maybe if the type of &classInstance.method was actually a special "pointer-to-class-method" type that implicitly down-casted to a delegate, it would be possible to safely use &o.watch instead...

I agree. Is the only reason to have a weak connection? -- /Jacob Carlborg
Jan 07 2014
parent reply Martin Nowak <code dawg.eu> writes:
On 01/11/2014 08:13 PM, Robert wrote:
 I agree. Is the only reason to have a weak connection?

Yes. Weak connections are the only reason.

Wouldn't it be possible to find out whether the delegate context ptr is actually an object? Not sure how to do it safely though and Interfaces slightly differ. ```d import std.stdio; class Foo { void method() {} } void main() { auto foo = new Foo; auto dg = &foo.method; // It's a class delegate!!! writeln(cast(void*)foo.classinfo.__vptr, " ", ***cast(void****)dg.ptr, " ", (new ClassInfo).__vptr); assert(***cast(void****)dg.ptr is (new ClassInfo).__vptr); } ```
Jan 23 2014
parent reply Jacob Carlborg <doob me.com> writes:
On 2014-01-23 23:51, Martin Nowak wrote:

 Wouldn't it be possible to find out whether the delegate context ptr
 is actually an object? Not sure how to do it safely though and
 Interfaces slightly differ.

 ```d
 import std.stdio;

 class Foo
 {
      void method() {}
 }

 void main()
 {
      auto foo = new Foo;
      auto dg = &foo.method;
      // It's a class delegate!!!
      writeln(cast(void*)foo.classinfo.__vptr, " ",
 ***cast(void****)dg.ptr, " ", (new ClassInfo).__vptr);
      assert(***cast(void****)dg.ptr is (new ClassInfo).__vptr);
 }
 ```

Why don't you just cast the delegate context pointer to Object? Like this: auto result = cast(Object) dg.ptr; If "result" is not null the context pointer points to an object. Although this won't handled interfaces. I consider it a bug that an interface cannot be casted to Object. -- /Jacob Carlborg
Jan 24 2014
next sibling parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"David Nadlinger"  wrote in message 
news:nbvmhmhcszmsruszggul forum.dlang.org...
 Although this won't handled interfaces. I consider it a bug that an 
 interface cannot be casted to Object.

Not all interface implementations are (Object) classes, cf. IUnknown.

This is not a problem, it is known at compile time.
Jan 24 2014
parent Jacob Carlborg <doob me.com> writes:
On 2014-01-24 12:52, Daniel Murphy wrote:

 This is not a problem, it is known at compile time.

Exactly. -- /Jacob Carlborg
Jan 24 2014
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2014-01-24 10:06, Johannes Pfau wrote:

 A delegate context could be a struct or closure as well, then calling
 cast(Object) is invalid and may crash.
 There's no good solution, the old std.signals just assumes all
 delegates belong to classes IIRC.

I just tried with a nested function and that returned "null". Was that just luck? -- /Jacob Carlborg
Jan 24 2014
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2014-01-24 12:13, David Nadlinger wrote:

 No, this is just a plain (i.e. no-op) cast.

It seemed to work when I tested it. Ok, I see what I did wrong. I forgot to actually refer to an outer variable in my nested function. -- /Jacob Carlborg
Jan 24 2014
prev sibling next sibling parent reply Martin Nowak <code dawg.eu> writes:
On 01/06/2014 11:39 AM, Dicebot wrote:
 =/ Any good idea to make those more visible? Maybe we should setup a
 mailing list for those who want to be notified about ongoing reviews?

Yes, a separate newsgroup would help here. Do you have an idea for a name? digitalmars.D.phobos or digitalmars.D.review
Jan 23 2014
next sibling parent Jacob Carlborg <doob me.com> writes:
On 2014-01-23 19:26, Dicebot wrote:

 digitalmars.D.review is better. We only review Phobos module that way
 now but it can be extended.

 Related offtopic:

 I wonder sometimes if it can/should be generalized for any important
 decision making involving community. Currently only Phobos reviews live
 with their own somewhat independent life, everything else is process
 bottleneck mandating some intervention from small core team. Probably
 investing into some tooling here can result in a great benefit.

I agree. There are also many DIP's which are just sitting in the wiki. -- /Jacob Carlborg
Jan 23 2014
prev sibling parent Martin Nowak <code dawg.eu> writes:
On 01/23/2014 07:26 PM, Dicebot wrote:
 I wonder sometimes if it can/should be generalized for any important
 decision making involving community. Currently only Phobos reviews live
 with their own somewhat independent life, everything else is process
 bottleneck mandating some intervention from small core team. Probably
 investing into some tooling here can result in a great benefit.

I would have really like more thorough review for the shared library pull request, although I'd like more code review in general. https://github.com/D-Programming-Language/druntime/pull/617
Jan 23 2014
prev sibling next sibling parent Martin Nowak <code dawg.eu> writes:
On 01/07/2014 09:18 AM, Robert wrote:
 I did, but there are still some bugs with template mixins, which finally
 drove me to a string mixin and in the end I liked it better.

Please file any bugs you encounter (https://d.puremagic.com/issues/). Most bugs can be minimized automatically using dustmite.
Jan 23 2014
prev sibling parent reply Martin Nowak <code dawg.eu> writes:
On 01/06/2014 11:13 AM, Vladimir Panteleev wrote:
 IMO, the effort to disallow third parties to "emit" signals on your
 object by far exceeds its worth.

 For this purpose, you have needed:
 - string mixins
    - ...which declare, by convention, a field with a name not explicitly
 given by the user
    - ...with a workaround for when they don't work
 - an enumeration type
 - two different signal struct types
 - a boatload of documentation
 - a nontrivial cognitive effort on the user to grok all of the above

 This little feature steals too much focus, and IMO is not worth it.

I absolutely agree. Ideally the module std.signal would consists of a template struct with 3 overloaded methods. struct Signal(Args...) { void connect(void delegate(Args) dg); void connect(WeakDelegate!(Args) dg); void disconnect(void delegate(Args) dg); void disconnect(WeakDelegate!(Args) dg); void emit(Args); } Still, looking at the documentation (https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html) and implementation you're somewhat off. Of course there are a few implementation issues. - There is no WeakDelegate in druntime or phobos. Maybe requiring explicit disconnect is good enough? - The lifetime of the delegate context could be scoped. No idea how to solve this one, but you could require that the delegate context is on the GC heap. - When used this Signal definition requires boilerplate to restrict access to emit. This is unlucky but doesn't justify doubling the complexity of the module. -Martin
Jan 23 2014
parent Martin Nowak <code dawg.eu> writes:
On 01/23/2014 06:16 PM, Martin Nowak wrote:
 Of course there are a few implementation issues.
 - There is no WeakDelegate in druntime or phobos.

    Maybe requiring explicit disconnect is good enough?

 - The lifetime of the delegate context could be scoped.

    No idea how to solve this one, but you could require that
    the delegate context is on the GC heap.

 - When used this Signal definition requires boilerplate
    to restrict access to emit.

    This is unlucky but doesn't justify doubling the complexity
    of the module.

BTW, this is the reason why I didn't participate in the review. We need to think harder about solving these issues without degrading the API. I am simply lacking the time to get into this, but I'm sure that a better trade-off can be found.
Jan 23 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev 
wrote:
 Apologies, I missed this review thread.

=/ Any good idea to make those more visible? Maybe we should setup a mailing list for those who want to be notified about ongoing reviews?
Jan 06 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Monday, 6 January 2014 at 10:39:41 UTC, Dicebot wrote:
 On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev 
 wrote:
 Apologies, I missed this review thread.

=/ Any good idea to make those more visible? Maybe we should setup a mailing list for those who want to be notified about ongoing reviews?

Yes, I also think that we should have separate mailing list for reviews and votings. At least, it will be easier to find previous reviews.
Jan 06 2014
prev sibling next sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Monday, 6 January 2014 at 10:39:41 UTC, Dicebot wrote:
 On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev 
 wrote:
 Apologies, I missed this review thread.

=/ Any good idea to make those more visible? Maybe we should setup a mailing list for those who want to be notified about ongoing reviews?

Well, it's my fault, really - I was not following D development at the time, as I was visiting relatives abroad - and if voting would have started after 2 weeks of review, I probably would not had been able to participate there either.
Jan 06 2014
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev 
wrote:
 On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
 Time to move forward with some more reviews :)

Hi, Apologies, I missed this review thread. So, some questions / observations: IMO, the effort to disallow third parties to "emit" signals on your object by far exceeds its worth. For this purpose, you have needed: - string mixins - ...which declare, by convention, a field with a name not explicitly given by the user - ...with a workaround for when they don't work - an enumeration type - two different signal struct types - a boatload of documentation - a nontrivial cognitive effort on the user to grok all of the above This little feature steals too much focus, and IMO is not worth it. As a compromise, have you considered template mixins?

I did, but there are still some bugs with template mixins, which finally drove me to a string mixin and in the end I liked it better. IMO there is no real benefit of a template mixin over a string mixin in this case. The implementation got a lot easier and bug free. Instead of dropping it all together, I think I will just move it to the bottom and make it a goody, instead of the proposed default usage of std.signal.
 This implementation supersedes the former std.signals

I don't think comparisons of older versions of a module belong in the module's documentation.

You are probably right, will fix that.
 dg must not be null. (Its context may.)

I don't think the context will ever be null (unless you take the address of a null class/struct pointer's method). (toDelegate uses the context pointer to store the function pointer, and the function pointer to store a template instance according to the function's arguments.)

It is for me, if I create a lamda delegate which does not use it's context. But I might just cut this from the documentation.
 It implements the emit function for all other functionality it 
 has this aliased to RestrictedSignal.

Missing some punctuation, I think.

Fixed, thanks!
 The idea is to instantiate a Signal privately and provide a 
 public accessor method for accessing the contained 
 RestrictedSignal.

I had a hard time understanding this line the first time. Please reword without using "The idea is", because it's not clear what "the idea" is for - usage? Rationalization of implementation method?
 Use this overload if you either really, really want strong ref 
 semantics for some reason

Was the emphasis really necessary? It makes complete sense to build an asynchronous-event-model application using 100% only "strong connections" (delegates or delegate arrays). I don't remember ever NEEDING weak-referenced callbacks.

You are right, but you already imply the reason in your argument, if you need strong connections for your application, you are probably better off with a simple delegate array. From discussions on this mailinglist it became apparent that people often use strong connections for no good reasons, nevertheless I just dropped it :-) Thanks a lot for this valuable feedback! Best regards, Robert
Jan 07 2014
prev sibling next sibling parent =?iso-8859-1?Q?Robert_M._M=FCnch?= <robert.muench saphirion.com> writes:
On 2013-11-07 13:45:56 +0000, Dicebot said:

 Our current victim is std.signal by Robert Klotzner which is supposed 
 to deprecate and supersede existing (and broken) std.signals

Hi, not really a comment regarding the actual implementation but I think that good debug support for signales & slots helps a lot in using it. What do I mean with this: - a way to dump in a human readable form the run-time connections. Which function / class / etc. is currently attached to which signal? - automatic logging like a call-stack in a debugger to get an idea when which signal is acted on - a way to get the order of activation for debugging to identify unwanted side-effects - etc. Big signal & slot implementaitons can be hard to debug, this should be as simple as possible. -- Robert M. Mnch Saphirion AG http://www.saphirion.com smarter | better | faster
Jan 11 2014
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
Definitely a good thing to have. But nothing that could not be 
added later on, for now I just hope there will be enough people 
voting, so it can actually be included in phobos.

If your only concern is additional features and nothing that is 
wrong with the API, consider voting.

I added the issue to phobosx for now:

https://github.com/phobos-x/phobosx/issues/4

Best regards,

Robert

On Saturday, 11 January 2014 at 18:02:15 UTC, Robert M. Münch 
wrote:
 On 2013-11-07 13:45:56 +0000, Dicebot said:

 Our current victim is std.signal by Robert Klotzner which is 
 supposed to deprecate and supersede existing (and broken) 
 std.signals

Hi, not really a comment regarding the actual implementation but I think that good debug support for signales & slots helps a lot in using it. What do I mean with this: - a way to dump in a human readable form the run-time connections. Which function / class / etc. is currently attached to which signal? - automatic logging like a call-stack in a debugger to get an idea when which signal is acted on - a way to get the order of activation for debugging to identify unwanted side-effects - etc. Big signal & slot implementaitons can be hard to debug, this should be as simple as possible.

Jan 11 2014
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
 This is really ugly, but I don't see any way to improve it 
 either. Maybe
 if the type of &classInstance.method was actually a special
 "pointer-to-class-method" type that implicitly down-casted to a
 delegate, it would be possible to safely use &o.watch 
 instead...

I agree. Is the only reason to have a weak connection?

Yes. Weak connections are the only reason.
Jan 11 2014
prev sibling next sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Tuesday, 7 January 2014 at 08:18:26 UTC, Robert wrote:
 Instead of dropping it all together, I think I will just move 
 it to the bottom and make it a goody, instead of the proposed 
 default usage of std.signal.

[...]
 You are probably right, will fix that.

Has this been done? I don't see any changes on https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html
 It is for me, if I create a lamda delegate which does not use 
 it's context.

Right, I forgot about that case.
Jan 11 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Sunday, 12 January 2014 at 01:06:45 UTC, Vladimir Panteleev 
wrote:
 On Tuesday, 7 January 2014 at 08:18:26 UTC, Robert wrote:
 Instead of dropping it all together, I think I will just move 
 it to the bottom and make it a goody, instead of the proposed 
 default usage of std.signal.

[...]
 You are probably right, will fix that.

Has this been done? I don't see any changes on https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html

Look this page: https://vhios.dyndns.org/dlang.org/web/phobos-prerelease/std_signal.html Is it OK?
Jan 11 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Saturday, 11 January 2014 at 18:02:15 UTC, Robert M. Münch 
wrote:
 Hi, not really a comment regarding the actual implementation 
 but I think that good debug support for signales & slots helps 
 a lot in using it. What do I mean with this:

 - a way to dump in a human readable form the run-time 
 connections. Which function / class / etc. is currently 
 attached to which signal?
 - automatic logging like a call-stack in a debugger to get an 
 idea when which signal is acted on
 - a way to get the order of activation for debugging to 
 identify unwanted side-effects
 - etc.

 Big signal & slot implementaitons can be hard to debug, this 
 should be as simple as possible.

Yes, it will be good to have. Also, you can vote `yes with condition` and write your condition after it (I think this description is good). In this case Robert receives your vote only if he implement your condition. It looks like not so many people use signals, so any feedback are welcome.
Jan 12 2014
prev sibling next sibling parent reply Dmitry Olshansky <dmitry.olsh gmail.com> writes:
07-Nov-2013 17:45, Dicebot пишет:
 Time to move forward with some more reviews :)

 Our current victim is std.signal by Robert Klotzner which is supposed to
 deprecate and supersede existing (and broken) std.signals

 If there are any frequent Phobos contributors / core developers
 please pay extra attention to submission code style and fitting
 into overall Phobos guidelines and structure.

There was a concern about the lack of feedback. Here goes. I'm not going to vote on this for the following reasons: 1. Never was fond of signal-slots as a pattern (Observer). I always find implementations to be overly complex for their own good. 2. When I needed something remotely like that I'll do just fine with array of delegates. 3. I never had no interest in old std.signal, I won't shed a tear if all kinds of std.signal would be removed from Phobos. 4. Even though I'd wanted to review the code itself I never found time to do it. -- Dmitry Olshansky
Jan 13 2014
parent reply Martin Nowak <code dawg.eu> writes:
On 01/13/2014 10:16 PM, ilya-stromberg wrote:
 It's not so good to have array of delegates because you will have a
 memory leaks. Delegate has permanent pointer to the object, so GC will
 never free it. As alternative, you can delete delegate manually, but in
 this case you can have problems with manual memory management.

 So, array of delegates is possible solution, but only for VERY simple
 cases. This module solves problem with memory leaks because it
 implements weak connections (based on weak reference). It's easy to use,
 but VERY difficult to implement.

But weak references should be implemented on GC/druntime level.
Jan 23 2014
parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
24-Jan-2014 02:58, Martin Nowak пишет:
 On 01/13/2014 10:16 PM, ilya-stromberg wrote:
 It's not so good to have array of delegates because you will have a
 memory leaks. Delegate has permanent pointer to the object, so GC will
 never free it. As alternative, you can delete delegate manually, but in
 this case you can have problems with manual memory management.

 So, array of delegates is possible solution, but only for VERY simple
 cases. This module solves problem with memory leaks because it
 implements weak connections (based on weak reference). It's easy to use,
 but VERY difficult to implement.

But weak references should be implemented on GC/druntime level.

It could be interesting to provide weak memory mapping with GC. Could be very useful for caches. -- Dmitry Olshansky
Jan 24 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Monday, 13 January 2014 at 18:16:33 UTC, Dmitry Olshansky 
wrote:
 2. When I needed something remotely like that I'll do just fine 
 with array of delegates.

It's not so good to have array of delegates because you will have a memory leaks. Delegate has permanent pointer to the object, so GC will never free it. As alternative, you can delete delegate manually, but in this case you can have problems with manual memory management. So, array of delegates is possible solution, but only for VERY simple cases. This module solves problem with memory leaks because it implements weak connections (based on weak reference). It's easy to use, but VERY difficult to implement. So, if you have any critical comments, please let us know. At least, Robert will have a chance to fix it.
Jan 13 2014
prev sibling next sibling parent reply "Kagamin" <spam here.lot> writes:
1. there're two typical usage patterns of signals: you mostly 
have 0 or 1 slots attached, so it would be nice if having one 
slot wouldn't require allocation. SlotsArray should be probably a 
tagged union of SlotImpl and SlotImpl[]. Is it worth an effort?
2. why InvisibleAddress is so big? Isn't it enough to just negate 
it?
3. I think, it's important to document, that a weak connection 
can be destroyed before it becomes logically unneeded. It's an 
important and non-trivial caveat.
Jan 17 2014
parent Jacob Carlborg <doob me.com> writes:
On 2014-01-19 11:30, Kagamin wrote:
 Also a similar warning for strongConnect: if you subscribe a big object
 (e.g. a complex UI form) to a signal and forget to unsubscribe, it will
 be retained with all the referenced data. In C# we register
 subscriptions in a supplementary container, so that when the form is
 disposed, it also disposes subscriptions in the container - it makes
 unsubscription easier if you don't forget to dispose the form.

It's recommended to use weak reference for IBOutles in OS X and iOS programming. IBOutles are used to hold reference to GUI objects instantiated in .nib files. -- /Jacob Carlborg
Jan 19 2014
prev sibling next sibling parent "Robert" <jfanatiker gmx.at> writes:
On Friday, 17 January 2014 at 08:46:33 UTC, Kagamin wrote:
 1. there're two typical usage patterns of signals: you mostly 
 have 0 or 1 slots attached, so it would be nice if having one 
 slot wouldn't require allocation. SlotsArray should be probably 
 a tagged union of SlotImpl and SlotImpl[]. Is it worth an 
 effort?

Well it depends. One of my goals was to minimize memory usage of an empty signal, using a tagged union would increase the space used by an empty signal. (A slot consists of 3 pointers + memory for the tag, the array consists of a pointer and a length)
 2. why InvisibleAddress is so big? Isn't it enough to just 
 negate it?

For 64 bit yes, but not for 32 bit. InvisibleAddress handles both.
 3. I think, it's important to document, that a weak connection 
 can be destroyed before it becomes logically unneeded. It's an 
 important and non-trivial caveat.

How is this supposed to happen?
Jan 17 2014
prev sibling next sibling parent "Kagamin" <spam here.lot> writes:
On Friday, 17 January 2014 at 13:31:10 UTC, Robert wrote:
 On Friday, 17 January 2014 at 08:46:33 UTC, Kagamin wrote:
 1. there're two typical usage patterns of signals: you mostly 
 have 0 or 1 slots attached, so it would be nice if having one 
 slot wouldn't require allocation. SlotsArray should be 
 probably a tagged union of SlotImpl and SlotImpl[]. Is it 
 worth an effort?

Well it depends. One of my goals was to minimize memory usage of an empty signal, using a tagged union would increase the space used by an empty signal. (A slot consists of 3 pointers + memory for the tag, the array consists of a pointer and a length)

We can infer the tag from one of the pointers and use two other for the array. I suppose, _funcPtr can't be null in a meaningfully filled slot, so if it's not null, that's a slot, otherwise _dataPtr and _obj are array's ptr and length respectively. So it's only 1.5 bigger.
 2. why InvisibleAddress is so big? Isn't it enough to just 
 negate it?

For 64 bit yes, but not for 32 bit. InvisibleAddress handles both.

False pointers. I'm afraid, 32-bit has them any way.
 3. I think, it's important to document, that a weak connection 
 can be destroyed before it becomes logically unneeded. It's an 
 important and non-trivial caveat.

How is this supposed to happen?

If you attach an object to a slot and then lose the last reference to it.
Jan 17 2014
prev sibling next sibling parent "Kagamin" <spam here.lot> writes:
Also a similar warning for strongConnect: if you subscribe a big 
object (e.g. a complex UI form) to a signal and forget to 
unsubscribe, it will be retained with all the referenced data. In 
C# we register subscriptions in a supplementary container, so 
that when the form is disposed, it also disposes subscriptions in 
the container - it makes unsubscription easier if you don't 
forget to dispose the form.
Jan 19 2014
prev sibling next sibling parent "Kagamin" <spam here.lot> writes:
That warning is for strongConnect.
Jan 20 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Thursday, 23 January 2014 at 16:17:38 UTC, Martin Nowak wrote:
 On 01/06/2014 11:39 AM, Dicebot wrote:
 =/ Any good idea to make those more visible? Maybe we should 
 setup a
 mailing list for those who want to be notified about ongoing 
 reviews?

Yes, a separate newsgroup would help here. Do you have an idea for a name? digitalmars.D.phobos or digitalmars.D.review

digitalmars.D.review is better. We only review Phobos module that way now but it can be extended. Related offtopic: I wonder sometimes if it can/should be generalized for any important decision making involving community. Currently only Phobos reviews live with their own somewhat independent life, everything else is process bottleneck mandating some intervention from small core team. Probably investing into some tooling here can result in a great benefit.
Jan 23 2014
prev sibling next sibling parent "robert" <jfanatiker gmx.at> writes:
 (https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html)

Was outdated (web/phobos-prerelase/std_signal.html was current), but the old link you posted is now also up2date. Sorry for that. Best regards, Robert
Jan 23 2014
prev sibling next sibling parent "ilya-stromberg" <ilya-stromberg-2009 yandex.ru> writes:
On Thursday, 23 January 2014 at 22:58:08 UTC, Martin Nowak wrote:
 On 01/13/2014 10:16 PM, ilya-stromberg wrote:
 It's not so good to have array of delegates because you will 
 have a
 memory leaks. Delegate has permanent pointer to the object, so 
 GC will
 never free it. As alternative, you can delete delegate 
 manually, but in
 this case you can have problems with manual memory management.

 So, array of delegates is possible solution, but only for VERY 
 simple
 cases. This module solves problem with memory leaks because it
 implements weak connections (based on weak reference). It's 
 easy to use,
 but VERY difficult to implement.

But weak references should be implemented on GC/druntime level.

Yes, I know. They can be useful for many other purposes. I just provide example when `std.signal` better than `array of delegates`.
Jan 23 2014
prev sibling next sibling parent Johannes Pfau <nospam example.com> writes:
Am Fri, 24 Jan 2014 09:52:23 +0100
schrieb Jacob Carlborg <doob me.com>:

 On 2014-01-23 23:51, Martin Nowak wrote:
 
 Wouldn't it be possible to find out whether the delegate context ptr
 is actually an object? Not sure how to do it safely though and
 Interfaces slightly differ.

 ```d
 import std.stdio;

 class Foo
 {
      void method() {}
 }

 void main()
 {
      auto foo = new Foo;
      auto dg = &foo.method;
      // It's a class delegate!!!
      writeln(cast(void*)foo.classinfo.__vptr, " ",
 ***cast(void****)dg.ptr, " ", (new ClassInfo).__vptr);
      assert(***cast(void****)dg.ptr is (new ClassInfo).__vptr);
 }
 ```

Why don't you just cast the delegate context pointer to Object? Like this: auto result = cast(Object) dg.ptr; If "result" is not null the context pointer points to an object. Although this won't handled interfaces. I consider it a bug that an interface cannot be casted to Object.

A delegate context could be a struct or closure as well, then calling cast(Object) is invalid and may crash. There's no good solution, the old std.signals just assumes all delegates belong to classes IIRC. Why do we even need this auto-detection? Just add an additional connectWeak function which either just takes a normal delegate and require that all delegates passed to it have objects as context. Or better: add a proper WeakRef type to druntime/GC as discussed before, then use overloads.
Jan 24 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Thursday, 23 January 2014 at 22:37:25 UTC, Martin Nowak wrote:
 I would have really like more thorough review for the shared 
 library pull request, although I'd like more code review in 
 general.
 https://github.com/D-Programming-Language/druntime/pull/617

We can do it, sure. You should provide some docs / functionality overview to conform usual reviewing requirements.
Jan 24 2014
prev sibling parent "David Nadlinger" <code klickverbot.at> writes:
On Friday, 24 January 2014 at 08:52:24 UTC, Jacob Carlborg wrote:
 Why don't you just cast the delegate context pointer to Object? 
 Like this:

 auto result = cast(Object) dg.ptr;

 If "result" is not null the context pointer points to an object.

No, this is just a plain (i.e. no-op) cast.
 Although this won't handled interfaces. I consider it a bug 
 that an interface cannot be casted to Object.

Not all interface implementations are (Object) classes, cf. IUnknown. David
Jan 24 2014