Review of std.signal

[Robert]

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