-
Notifications
You must be signed in to change notification settings - Fork 29
feat: Add Swift 6 language mode support #920
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
… an error (#921) Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
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.
Ignore this
Package.resolved
Outdated
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.
Ignore this it will be removed
Package.swift
Outdated
@@ -56,7 +56,7 @@ let package = Package( | |||
], | |||
dependencies: { | |||
var dependencies: [Package.Dependency] = [ | |||
.package(url: "https://github.com/awslabs/aws-crt-swift.git", exact: "0.48.0"), | |||
.package(url: "https://github.com/awslabs/aws-crt-swift.git", branch: "swift6-support"), |
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.
ignore this
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.
6 Qs & 1 suggestion
Sources/ClientRuntime/Config/DefaultSDKRuntimeConfiguration.swift
Outdated
Show resolved
Hide resolved
Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
Outdated
Show resolved
Hide resolved
Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
Outdated
Show resolved
Hide resolved
Package.resolved
Outdated
@@ -0,0 +1,33 @@ | |||
{ |
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.
Package.resolved should be deleted from this PR before merge
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.
Yes it will be
private func take() -> CheckedContinuation<Response, Error>? { | ||
lock.lock() | ||
defer { lock.unlock() } | ||
defer { continuation = nil } |
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.
Multiple defer
statements execute in reverse order they are written, so this is correct, but for simplicity, why not just use one defer with two lines and return a temp var?
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 can change it to that if its preferred 👍 -- I wrote out both options and liked this one more.
|
||
public struct Endpoint: Hashable { | ||
public struct Endpoint: Sendable, Equatable { |
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.
So we were using Endpoint equality but not hashability?
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.
Yeah we were never hashing this but I assume we marked it as Hashable since properties had [String: AnyHashable]
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.
Do we actually use endpoint equality though?
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.
Its used here
Issue #
Description of changes
properties
now is of typeEndpointProperty
as defined by CRT. A wrapperEndpointPropertyValue
is defined to obfuscate CRT dependencyScope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.