Skip to content

[Driver] Refactor ToolChains into separate files. #16091

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 1 commit into from
May 2, 2018

Conversation

troughton
Copy link
Contributor

@troughton troughton commented Apr 22, 2018

This PR refactors ToolChains.cpp into four separate files: the platform-specific UnixToolChains.cpp, DarwinToolChains.cpp, and WindowsToolChains.cpp, along with keeping the general-purpose ToolChains.cpp.

It also factors together some shared functionality, and I've applied clang-format to all affected files since the formatting was previously inconsistent.

Note that this PR does require moving away from free functions in ToolChains.cpp; I've instead added them to the ToolChain base class.

No functionality change intended.

@jrose-apple As discussed in #13236

@compnerd
Copy link
Member

Nice, thanks for doing this!

@compnerd compnerd requested a review from jrose-apple April 24, 2018 00:09
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Definite improvement. Thanks, Thomas!

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple jrose-apple self-assigned this May 1, 2018
@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2018

Build failed
Swift Test OS X Platform
Git Sha - 9f20a6832caa6ee3b10e628576c4c438fa6c3cf0

@troughton
Copy link
Contributor Author

Looks like CI hasn't picked up the changes to the CMakeLists file.

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2018

Build failed
Swift Test Linux Platform
Git Sha - 9f20a6832caa6ee3b10e628576c4c438fa6c3cf0

@jrose-apple
Copy link
Contributor

No, the case is wrong! *ToolChains.cpp vs. *Toolchains.cpp.

@troughton troughton force-pushed the toolchains-refactor branch from 9f20a68 to 21e5e93 Compare May 1, 2018 03:12
@troughton
Copy link
Contributor Author

troughton commented May 1, 2018

My mistake - but wow, that's a trap. The case was actually right on my local filesystem, but because I was working on macOS, git config defaults to being case insensitive (core.ignorecase). My first commit had the wrong case in the naming for the *ToolChains.cpp files, and when I fixed it and amended the commit Git didn't pick up the change.

@jrose-apple
Copy link
Contributor

That's what PR testing is for! I've done it too, don't worry.

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2018

Build failed
Swift Test Linux Platform
Git Sha - 9f20a6832caa6ee3b10e628576c4c438fa6c3cf0

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2018

Build failed
Swift Test OS X Platform
Git Sha - 9f20a6832caa6ee3b10e628576c4c438fa6c3cf0

Format the code and factor together some common functionality at the same time.
@troughton troughton force-pushed the toolchains-refactor branch from 2ba5b99 to 82e7f68 Compare May 1, 2018 21:52
@troughton
Copy link
Contributor Author

@jrose-apple Could you please trigger CI testing again?

That time Git added the files rather than recognising them as a rename, and my local checkout was convinced there was only one copy. I had to use the web interface to actually delete the lowercased files.

@gottesmm
Copy link
Contributor

gottesmm commented May 1, 2018

@swift-ci Please test

1 similar comment
@gottesmm
Copy link
Contributor

gottesmm commented May 1, 2018

@swift-ci Please test

@jrose-apple jrose-apple merged commit 0506ccc into swiftlang:master May 2, 2018
@troughton troughton deleted the toolchains-refactor branch May 2, 2018 23:05
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.

5 participants