Skip to content
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

Merged
merged 2 commits into from
May 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Sources/Deprecations+Removals.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ extension ComposableMutablePropertyProtocol {
public func withValue<Result>(action: (Value) throws -> Result) rethrows -> Result { fatalError() }
}

extension CompositeDisposable {
@available(*, deprecated, message:"Use `Disposable?` instead. The typealias would be removed in a future release.")
public typealias DisposableHandle = Disposable?
}

extension Optional where Wrapped == Disposable {
@available(*, unavailable, renamed:"dispose")
public func remove() { fatalError() }
}

@available(*, unavailable, renamed:"SignalProducer.timer")
public func timer(interval: DispatchTimeInterval, on scheduler: DateScheduler) -> SignalProducer<Date, NoError> { fatalError() }

Expand Down Expand Up @@ -239,7 +249,7 @@ extension Bag {

extension CompositeDisposable {
@available(*, unavailable, renamed:"add(_:)")
public func addDisposable(_ d: Disposable) -> DisposableHandle { fatalError() }
public func addDisposable(_ d: Disposable) -> Disposable? { fatalError() }
}

extension Observer {
Expand Down
94 changes: 27 additions & 67 deletions Sources/Disposable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -110,48 +110,6 @@ public final class CompositeDisposable: Disposable {
private let disposables: Atomic<Bag<Disposable>?>
private var state: UnsafeAtomicState<DisposableState>

/// Represents a handle to a disposable previously added to a
/// `CompositeDisposable`.
///
/// - note: `add(_:)` method of `CompositeDisposable` creates instances of
/// `DisposableHandle`.
public final class DisposableHandle {
private var state: UnsafeAtomicState<DisposableState>
private var bagToken: Bag<Disposable>.Token?
private weak var disposable: CompositeDisposable?

fileprivate static let empty = DisposableHandle()

fileprivate init() {
self.state = UnsafeAtomicState(.disposed)
self.bagToken = nil
}

deinit {
state.deinitialize()
}

fileprivate init(bagToken: Bag<Disposable>.Token, disposable: CompositeDisposable) {
self.state = UnsafeAtomicState(.active)
self.bagToken = bagToken
self.disposable = disposable
}

/// Remove the pointed-to disposable from its `CompositeDisposable`.
///
/// - note: This is useful to minimize memory growth, by removing
/// disposables that are no longer needed.
public func remove() {
if state.tryDispose(), let token = bagToken {
_ = disposable?.disposables.modify {
$0?.remove(using: token)
}
bagToken = nil
disposable = nil
}
}
}

public var isDisposed: Bool {
return state.is(.disposed)
}
Expand Down Expand Up @@ -202,41 +160,43 @@ public final class CompositeDisposable: Disposable {
}
}

/// Add the given disposable to the list, then return a handle which can
/// be used to opaquely remove the disposable later (if desired).
/// Add the given disposable to the composite.
///
/// - parameters:
/// - d: Optional disposable.
/// - disposable: A disposable.
///
/// - returns: An instance of `DisposableHandle` that can be used to
/// opaquely remove the disposable later (if desired).
/// - returns: A disposable to remove `disposable` from the composite. `nil` if the
/// composite has been disposed of, `disposable` has been disposed of, or
/// `disposable` is `nil`.
@discardableResult
public func add(_ d: Disposable?) -> DisposableHandle {
guard let d = d else {
return DisposableHandle.empty
public func add(_ disposable: Disposable?) -> Disposable? {
Copy link
Contributor

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?

Copy link
Member Author

@andersio andersio May 10, 2017

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

guard let d = disposable, !d.isDisposed, !isDisposed else {
disposable?.dispose()
return nil
}

let handle: DisposableHandle? = disposables.modify {
return ($0?.insert(d)).map { DisposableHandle(bagToken: $0, disposable: self) }
}
return disposables.modify { disposables in
guard disposables != nil else { return nil }

if let handle = handle {
return handle
} else {
d.dispose()
return DisposableHandle.empty
let token = disposables!.insert(d)
return ActionDisposable { [weak self] in
self?.disposables.modify {
$0?.remove(using: token)
}
}
}
}

/// Add an ActionDisposable to the list.
/// Add the given action to the composite.
///
/// - parameters:
/// - action: A closure that will be invoked when `dispose()` is called.
/// - action: A closure to be invoked when the composite is disposed of.
///
/// - returns: An instance of `DisposableHandle` that can be used to
/// opaquely remove the disposable later (if desired).
/// - returns: A disposable to remove `disposable` from the composite. `nil` if the
/// composite has been disposed of, `disposable` has been disposed of, or
/// `disposable` is `nil`.
@discardableResult
public func add(_ action: @escaping () -> Void) -> DisposableHandle {
public func add(_ action: @escaping () -> Void) -> Disposable? {
return add(ActionDisposable(action: action))
}

Expand Down Expand Up @@ -351,7 +311,7 @@ public final class SerialDisposable: Disposable {
/// - returns: An instance of `DisposableHandle` that can be used to opaquely
/// remove the disposable later (if desired).
@discardableResult
public func +=(lhs: CompositeDisposable, rhs: Disposable?) -> CompositeDisposable.DisposableHandle {
public func +=(lhs: CompositeDisposable, rhs: Disposable?) -> Disposable? {
return lhs.add(rhs)
}

Expand All @@ -369,7 +329,7 @@ public func +=(lhs: CompositeDisposable, rhs: Disposable?) -> CompositeDisposabl
/// - returns: An instance of `DisposableHandle` that can be used to opaquely
/// remove the disposable later (if desired).
@discardableResult
public func +=(lhs: CompositeDisposable, rhs: @escaping () -> ()) -> CompositeDisposable.DisposableHandle {
public func +=(lhs: CompositeDisposable, rhs: @escaping () -> ()) -> Disposable? {
return lhs.add(rhs)
}

Expand All @@ -387,7 +347,7 @@ public func +=(lhs: CompositeDisposable, rhs: @escaping () -> ()) -> CompositeDi
/// - returns: An instance of `DisposableHandle` that can be used to opaquely
/// remove the disposable later (if desired).
@discardableResult
public func +=(lhs: ScopedDisposable<CompositeDisposable>, rhs: Disposable?) -> CompositeDisposable.DisposableHandle {
public func +=(lhs: ScopedDisposable<CompositeDisposable>, rhs: Disposable?) -> Disposable? {
return lhs.inner.add(rhs)
}

Expand All @@ -405,6 +365,6 @@ public func +=(lhs: ScopedDisposable<CompositeDisposable>, rhs: Disposable?) ->
/// - returns: An instance of `DisposableHandle` that can be used to opaquely
/// remove the disposable later (if desired).
@discardableResult
public func +=(lhs: ScopedDisposable<CompositeDisposable>, rhs: @escaping () -> ()) -> CompositeDisposable.DisposableHandle {
public func +=(lhs: ScopedDisposable<CompositeDisposable>, rhs: @escaping () -> ()) -> Disposable? {
return lhs.inner.add(rhs)
}
4 changes: 2 additions & 2 deletions Sources/Flatten.swift
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ extension Signal where Value: SignalProducerProtocol, Error == Value.Error {
signal.observe { event in
switch event {
case .completed, .interrupted:
handle.remove()
handle?.dispose()

let shouldComplete: Bool = state.modify { state in
state.activeCount -= 1
Expand Down Expand Up @@ -839,7 +839,7 @@ extension Signal where Value: SignalProducerProtocol, Error == Value.Error {

// Dispose all running innerSignals except winning one.
if !relayDisposable.isDisposed {
disposableHandle.remove()
disposableHandle?.dispose()
relayDisposable.dispose()
}

Expand Down
4 changes: 2 additions & 2 deletions Tests/ReactiveSwiftTests/DisposableSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ class DisposableSpec: QuickSpec {
let handle = disposable += simpleDisposable

// We should be allowed to call this any number of times.
handle.remove()
handle.remove()
handle?.dispose()
handle?.dispose()
expect(simpleDisposable.isDisposed) == false

disposable.dispose()
Expand Down