Skip to content

[5.0][stdlib][SR-2239]: add support for AArch64 variadics #20863

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

Closed
wants to merge 2 commits into from
Closed

[5.0][stdlib][SR-2239]: add support for AArch64 variadics #20863

wants to merge 2 commits into from

Conversation

futurejones
Copy link
Contributor

Cherry-pick of: #20862
Adds support for AArch64 variadics.

Resolves SR-2239.

Previous issues preventing building of the swift-5.0-branch have been fixed by defaulting to the "gold" linker.
This has been updated in #20846

We are now seeing successful builds of the swift-5.0-branch on AArch64 Linux Ubuntu 16.04
http://futurejones.xyz:8080/job/swift-5.0-aarch64/

@tkremenek
Copy link
Member

@swift-ci test

@tkremenek
Copy link
Member

This can land after #20877 is merged

Copy link
Contributor

@bob-wilson bob-wilson left a comment

Choose a reason for hiding this comment

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

The corresponding PR for master got a number of review comments. I couldn't tell if those concerns have been addressed (because the PR was merged before the review), but I think not. We should make sure we're happy with the changes and get any follow-up commits included here before merging this.

@futurejones
Copy link
Contributor Author

@bob-wilson @tkremenek I have assessed the comments of the reviewers of the PR for the master and I have found no issues that need to be addressed before this PR can be merged.

This code and the other related PR's have been thoroughly tested on the target platform before submitting.

Suggestions about refactoring code that effect platforms other than AArch64/Linux can be best addressed in future PR's.

@tkremenek
Copy link
Member

All PRs with the pending-convergence label are being held for merging. The swift-5.0-branch branch is being qualified and change is being throttled until we get the branch into a good baseline. That restriction should hopefully be lifted soon.

@bob-wilson
Copy link
Contributor

@futurejones Perhaps other open-source projects work differently, but at least for Swift, the expectation is that you will come to an agreement during the code review process. When I look at #20862 I see a bunch of requests for changes that you marked as "resolved" without either changing anything or convincing others to accept your opinion that those changes should be done later. That is not how it works. We really do want your contributions to Swift and I know it can be frustrating to be asked for follow-up changes when you're just trying to get it to work, but we also have to watch out for the long-term quality of the code. Please work with the reviewers to reach a consensus about what to do on the master branch. If that doesn't happen soon, I think we should revert the change from master until the review is completed.

@futurejones
Copy link
Contributor Author

@bob-wilson please see my comments on #20862
I hope this satisfies the requirements to get this PR merged.

@tkremenek can we move forward on this please.

@bob-wilson
Copy link
Contributor

I will defer to the reviewers of #21237 whether they think this should go in to the 5.0 branch with or without those changes (or perhaps a subset of them).

[Stdlib] Rename AnyKeyPath's ObjC name and _VaListBuilder to avoid conflicts with older stdlibs. #21127
@bob-wilson
Copy link
Contributor

Is this PR obsolete now that we've merged #21885?

@drodriguez
Copy link
Contributor

In my opinion, this one is obsolete after merging #21885 (thanks for that, btw).

@futurejones
Copy link
Contributor Author

@bob-wilson this PR is no longer needed as the changes have been included in #21885.
Thank you to everyone for getting these changes into swift-5.0-branch, especially @drodriguez.

@futurejones futurejones deleted the swift-5.0-aaarch64-VarArgs branch January 29, 2019 23:26
@AnthonyLatsis AnthonyLatsis added Linux Platform: Linux arm64 Architecture: arm64 (aarch64) — any 64-bit ARM and removed AArch64 Linux labels Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm64 Architecture: arm64 (aarch64) — any 64-bit ARM Linux Platform: Linux pending convergence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants