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.

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

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?

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 )

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_

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

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.

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.

> 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

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.