-
Notifications
You must be signed in to change notification settings - Fork 436
Remove swift-atomics from v2 #2041
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
Conversation
Motivation: Swift 6 includes built-in support for atomics in the Synchronization module. Currently we depend on the swift-atomics package which is no longer necessary. Modifications: Remove usages of swift-atomics from v2 Result: Fewer dependencies
import NIOCore | ||
import NIOHTTP2 | ||
import NIOPosix | ||
import Synchronization | ||
private import Synchronization | ||
import XCTest |
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 didn't enable internal imports by default on test targets and executable targets (and didn't bother adding the modifiers to all imports) because test/executable targets can't be dependencies anyways, so I don't think there's any benefit in using access level modifiers on them. It's a nit, but unless you disagree with this I'd just drop the private
. If you think it's worth having access levels across all (v2) targets, then I can enable it.
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.
That's fair, I did it out of habit. I'm fine not bothering in tests, so I'll remove this and elsewhere.
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 we've got a few utils copied between test targets - do you think it would make sense to have a test utils target to centralise some of the code? Or not worth it?
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 was wondering the same thing the other day, I want to hold off on making that decision until we figure out the package split situation.
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.
LGTM, thanks George! Tests particularly look much cleaner now with the AtomicCounter
😌
Motivation:
Swift 6 includes built-in support for atomics in the Synchronization module. Currently we depend on the swift-atomics package which is no longer necessary.
Modifications:
Remove usages of swift-atomics from v2
Result:
Fewer dependencies