-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove CompositeDisposable.DisposableHandle
.
#363
Conversation
Sources/Disposable.swift
Outdated
return DisposableHandle.empty | ||
return disposable.flatMap { disposable in | ||
return disposables.modify { disposables in | ||
let token = disposables!.insert(disposable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this !
is safe. This could have been disposed between the above check and here.
Sources/Disposable.swift
Outdated
} else { | ||
d.dispose() | ||
return DisposableHandle.empty | ||
return disposable.flatMap { disposable in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous guard
structure is preferable to disposable.flatMap
to reduce the nesting by a level.
public func add(_ d: Disposable?) -> DisposableHandle { | ||
guard let d = d else { | ||
return DisposableHandle.empty | ||
public func add(_ disposable: Disposable?) -> Disposable? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could leave this as non-optional by returning a disposable that does nothing. I'm not sure which would be better. Is there a particular reason you decided to return an optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It follows the precedence of Signal.observe
, and the fact that CompositeDisposable.add
accepts Disposable?
.
5d13e52
to
81d0409
Compare
The disposable array can be cleared at any time, since the disposal may happen after checking `isDisposed` but before accessing `disposables`.
81d0409
to
5ae1247
Compare
Cherry-picked from #334.
Use
ActionDisposable
instead, akin toSignal.observe
.