Skip to content

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

Merged
merged 37 commits into from
May 1, 2025
Merged

feat: Add Swift 6 language mode support #920

merged 37 commits into from
May 1, 2025

Conversation

dayaffe
Copy link
Contributor

@dayaffe dayaffe commented Apr 14, 2025

Issue #

Description of changes

  • Bumps CRT version from 0.48 to 0.52.1 and includes changes from chore: Bump CRT version to 0.51.0 from 0.48.0 #925
  • Refactor CRTClientEngine to break up long functions
  • Endpoint properties now is of type EndpointProperty as defined by CRT. A wrapper EndpointPropertyValue is defined to obfuscate CRT dependency
  • Add github actions workflow that modifies tools version to 6.0 and builds

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dayaffe
Copy link
Contributor Author

dayaffe commented Apr 17, 2025

Copy link
Contributor Author

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
Copy link
Contributor Author

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"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore this

Copy link
Contributor

@sichanyoo sichanyoo left a 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

@dayaffe dayaffe requested a review from sichanyoo April 21, 2025 19:11
@dayaffe dayaffe requested a review from jbelkins April 22, 2025 18:01
Package.resolved Outdated
@@ -0,0 +1,33 @@
{
Copy link
Contributor

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

Copy link
Contributor Author

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 }
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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]

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its used here

@dayaffe dayaffe requested review from jbelkins and sichanyoo April 30, 2025 14:58
@dayaffe dayaffe merged commit 2375537 into main May 1, 2025
32 checks passed
@dayaffe dayaffe deleted the day/swift6-support branch May 1, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants