Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

fix(scope): should allow removing listener during an event #695

Closed
wants to merge 1 commit into from

Conversation

pavelgj
Copy link
Contributor

@pavelgj pavelgj commented Mar 10, 2014

No description provided.

@pavelgj pavelgj added cla: yes and removed cla: no labels Mar 10, 2014
@@ -736,6 +736,9 @@ class ScopeStream extends async.Stream<ScopeEvent> {
final _Streams _streams;
final String _name;
final subscriptions = <ScopeStreamSubscription>[];
bool _firing = false;
List<ScopeStreamSubscription> _toRemove;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavelgj out of curiosity, why have you picked a lazy init here ? (the code could be simpler at the expense of some bytes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to avoid unnecessary allocations to handle a corner case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be a good use case for introducing a getter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I imagine a getter here it would also result in an unnecessary allocation when _remove is never called. How would you do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. I must be tired, I've just lost half a day fixing my ubuntu setup...

pavelgj added a commit that referenced this pull request Mar 12, 2014
pavelgj added a commit that referenced this pull request Mar 12, 2014
@pavelgj pavelgj closed this in 4662d49 Mar 13, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

2 participants