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

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.

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.

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?

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.

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 C# we register 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.

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
> be retained with all the referenced data. In C# we register
> 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

That warning is for strongConnect.

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

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.