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
