-
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
Streamlined the mechanics behind composed properties. #340
Conversation
5b4b2ce
to
a03fdf3
Compare
6408cd9
to
d1c58ff
Compare
Sources/Property.swift
Outdated
init(_ value: Value) { | ||
self.value = value | ||
} | ||
} |
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'd like to see this use the same approach as Atomic
to prevent unsafe accesses—hiding value
and lock
behind locking APIs.
Sources/Property.swift
Outdated
} | ||
|
||
extension NSRecursiveLock { | ||
fileprivate func sync<Result>(_ action: () throws -> Result) rethrows -> Result { |
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 this can go away of appropriate methods are added to PropertyBox
.
Sources/Property.swift
Outdated
fileprivate func sync<Result>(_ action: () throws -> Result) rethrows -> Result { | ||
lock(); defer { unlock() } | ||
return try action() | ||
func modifyDidSet<Result>(_ action: (inout Value) throws -> Result, didSet: (() -> Void)?) rethrows -> Result { |
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.
Just make this:
func modify<Result>(
_ action: (inout Value) throws -> Result,
didSet (() -> Void)? = nil
) rethrows -> Result {
?
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.
Ah, I thought the trailing closure would be in conflict. But I just realize didSet
can be moved as the first. 😛
Sources/Property.swift
Outdated
} | ||
}, didSet: { | ||
observer.action(event) |
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.
Can't observer.action
be in the action
block? It still happens inside the lock either way.
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 would violate access exclusivity because of the "de jure" copy-in, copy-out semantic of inout
being in conflict with the guarantee of property observers always seeing the latest value.
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.
Ah, MutableProperty.modify
needs to be changed too.
d27690e
to
4510c3b
Compare
b395238
to
af6a7f6
Compare
af6a7f6
to
35e1202
Compare
Sources/Atomic.swift
Outdated
// default. Since fairness is not a concern of our reactive primitives | ||
// anyway, it can be disabled. | ||
pthread_mutexattr_setpolicy_np(attr, _PTHREAD_MUTEX_POLICY_FIRSTFIT) | ||
#endif |
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 love setting an untested (in the real world) policy for only older Apple OSes.
Because this would affect a small portion of devices, it's hard to have confidence that it works well. I'd rather see us take a performance hit on older OS versions and not have a separate code path that's hard to test.
Replaced
replayLazily(upTo:)
with a custom implementation.Composed properties now cache only one copy of the latest value, down from 1-2 copies currently. Moreover, it now stops caching after both the
Property
and all the producer instances has deinitialised.