-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Conversation
Nice, thanks for doing 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.
Definite improvement. Thanks, Thomas!
@swift-ci Please test |
Build failed |
Looks like CI hasn't picked up the changes to the CMakeLists file. |
Build failed |
No, the case is wrong! *ToolChains.cpp vs. *Toolchains.cpp. |
9f20a68
to
21e5e93
Compare
My mistake - but wow, that's a trap. The case was actually right on my local filesystem, but because I was working on macOS, |
That's what PR testing is for! I've done it too, don't worry. @swift-ci Please test |
Build failed |
Build failed |
Format the code and factor together some common functionality at the same time.
2ba5b99
to
82e7f68
Compare
@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. |
@swift-ci Please test |
1 similar comment
@swift-ci Please test |
This PR refactors
ToolChains.cpp
into four separate files: the platform-specificUnixToolChains.cpp
,DarwinToolChains.cpp
, andWindowsToolChains.cpp
, along with keeping the general-purposeToolChains.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 theToolChain
base class.No functionality change intended.
@jrose-apple As discussed in #13236