-
Notifications
You must be signed in to change notification settings - Fork 218
SwiftDriver: add support for baremetal targets (to reflect https://github.com/apple/swift/pull/35970) #1313
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
|
Note to self: I need to add a testcase for this. |
| guard let os else { | ||
| diagnosticsEngine.emit(.error_unknown_target(triple)) | ||
| throw Driver.ErrorDiagnostics.emitted | ||
| } |
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.
Why the guard over the default case?
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'm guessing it's to avoid future reader ambiguity between Optional.none and OS.none.
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.
That would be helpful as a comment if that is the case. Doubly so since the handling is identical.
| case nil: | ||
| guard let os else { | ||
| fatalError("unknown OS") | ||
| } |
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.
Why not sink the fatalError into the default case?
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'd assume if default case is added to this switch, that would silence future compiler errors in this switch when a new case is added to Triple.OS.
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.
Sure, but you can handle that with .some and .none :)
90aa042 to
06c4244
Compare
06c4244 to
644e0f6
Compare
|
So apparently the "none" case was at least confusing but I'd say even misleading. Changed it to "noneOS" to avoid the clash with Optional.none. Added tests. |
|
@swift-ci please test |
compnerd
left a comment
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.
Thank you for porting the work from the c++ driver! I know I'd been slow to finish up the work :(
MaxDesiatov
left a comment
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.
yesplz, I look forward to trying it out!
|
Is it worth retrying the Windows job? Couldn't find the exact failure reason for it. |
|
Seems to have timed out. I think that the machine sometimes is under higher load. |
|
@swift-ci please test windows platform |
No description provided.