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


Just one question. When I looked on code I noticed this part 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?
Nov 07 2013
next sibling parent reply "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


 Just one question. When I looked on code I noticed this part 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 )
Nov 07 2013
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


 Just one question. When I looked on code I noticed this part 
 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 parent "Robert" <jfanatiker gmx.at> writes:

 Just one question. When I looked on code I noticed this part 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?


Thanks, was an oversight. Fixed in master.



 Nov 08 2013



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



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



  parent reply "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



  parent reply "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



 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  parent reply "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



  parent reply "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



  parent reply "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



  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  parent reply "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



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



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



  parent reply "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



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



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



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



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



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



  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 reply "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



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



 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  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 reply =?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. Münch
Saphirion AG

http://www.saphirion.com
smarter | better | faster



 Jan 11 2014



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


[snip]

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



  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



 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  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  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 reply "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



 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  parent reply "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 

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



  parent reply 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

 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



  parent  "Kagamin" <spam here.lot>  writes:


That warning is for strongConnect.



 Jan 20 2014