Skip to content
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

SwiftDriver: add support for baremetal targets (to reflect https://github.com/apple/swift/pull/35970) #1313

Merged
merged 1 commit into from
Apr 1, 2023

Conversation

kubamracek
Copy link
Contributor

No description provided.

@kubamracek kubamracek requested review from compnerd and artemcm March 21, 2023 16:43
@kubamracek
Copy link
Contributor Author

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

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?

Copy link
Member

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.

Copy link
Member

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.

fatalError("unknown OS")
}
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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 :)

@kubamracek kubamracek force-pushed the mracek/add-baremetal-target branch from 90aa042 to 06c4244 Compare March 31, 2023 02:29
@kubamracek kubamracek force-pushed the mracek/add-baremetal-target branch from 06c4244 to 644e0f6 Compare March 31, 2023 02:31
@kubamracek
Copy link
Contributor Author

kubamracek commented Mar 31, 2023

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.

@kubamracek
Copy link
Contributor Author

@swift-ci please test

Copy link
Member

@compnerd compnerd left a 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 :(

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a 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!

@MaxDesiatov
Copy link
Contributor

Is it worth retrying the Windows job? Couldn't find the exact failure reason for it.

@compnerd
Copy link
Member

Seems to have timed out. I think that the machine sometimes is under higher load.

@compnerd
Copy link
Member

@swift-ci please test windows platform

@kubamracek kubamracek merged commit 7a96ffd into main Apr 1, 2023
@kubamracek kubamracek deleted the mracek/add-baremetal-target branch April 1, 2023 04:01
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