Review of std.signal

[Vladimir Panteleev]

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.