-
Notifications
You must be signed in to change notification settings - Fork 433
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
[SwiftSyntax] Add #_objectFileFormat compilation conditional #3027
Conversation
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.
Should this also be guarded behind and experimental feature?
Edit: This should be guarded behind an experimental feature.
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.
Regarding the experimental feature flag, we currently don't have infrastructure to handle feature flags in SwiftIfConfig
, and I don't think we should include such change in this specific PR. So IMO we should take this as-is, then establish the feature flag facility asap, if we want to hide this in a feature flag.
/// - Parameters: | ||
/// - name: The name of the object file format. | ||
/// - Returns: Whether the target object file format matches the given name. | ||
func isActiveTargetObjectFileFormat(name: String) throws -> Bool |
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.
Could you add a default implementation returning false
so that we can land this swift-syntax
PR independently before swift
repo counterpart?
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.
Added!
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.
The test failure is because the default implementation is not public
🙏
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.
We should also make this @_spi(ExperimentalLanguageFeatures)
so that clients can’t call it as public API and start relying on it.
As for the "format check" failure, please just do |
@swift-ci Please test |
/// - Parameters: | ||
/// - name: The name of the object file format. | ||
/// - Returns: Whether the target object file format matches the given name. | ||
func isActiveTargetObjectFileFormat(name: String) throws -> Bool |
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.
We should also make this @_spi(ExperimentalLanguageFeatures)
so that clients can’t call it as public API and start relying on it.
/// swift repo. | ||
extension BuildConfiguration { | ||
func isActiveTargetObjectFileFormat(name: String) throws -> Bool { | ||
return false |
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 think this method should throw an error that has a description like '_objectFileFormat' is an experimental feature
. That way _objectFileFormat
will not be allowed unless BuildConfiguration
actively implements it. This way we also don’t silently assume that there is no active target object file format, as we do right now.
The BuildConfiguration
implementation in ASTGen can then check whether the _objectFileFormat
experimental feature is enabled. If not, it can throw a similar error. And if the feature is enabled, it can return true
or false
.
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.
Done
/// Default implementation of BuildConfiguration, to avoid a revlock with the | ||
/// swift repo. |
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.
This is not only to avoid revlock with the compiler repo. It is also to not introduce a new requirement to BuildConfiguration
for an experimental feature that would be API breaking.
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.
Fixed comment
@@ -36,6 +36,7 @@ enum IfConfigDiagnostic: Error, CustomStringConvertible { | |||
case likelySimulatorPlatform(syntax: ExprSyntax) | |||
case likelyTargetOS(syntax: ExprSyntax, replacement: ExprSyntax?) | |||
case endiannessDoesNotMatch(syntax: ExprSyntax, argument: String) | |||
case objectFileFormatDoesNotMatch(syntax: ExprSyntax, argument: String) |
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.
Is this used anywhere?
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.
Removed
@swift-ci please test |
@swift-ci please test windows |
SwiftSyntax part for swiftlang/swift#80212.
rdar://147505228