-
Notifications
You must be signed in to change notification settings - Fork 440
[SwiftParser] Experimental feature flags for _borrow and _move #3018
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
53c6038
to
d1efcb9
Compare
swiftlang/swift#80046 |
3 similar comments
swiftlang/swift#80046 |
swiftlang/swift#80046 |
swiftlang/swift#80046 |
CodeGeneration/Sources/SyntaxSupport/ExperimentalFeatures.swift
Outdated
Show resolved
Hide resolved
_borrow and _move should be pased as and ownership operator only when a feature flag is enabled.
d1efcb9
to
64589f7
Compare
swiftlang/swift#80046 |
swiftlang/swift#80046 |
case (._borrow, let handle)?: | ||
assert(self.experimentalFeatures.contains(.oldOwnershipOperatorSpellings)) |
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 make these precondition
s?
swift-syntax/Sources/SwiftSyntax/Assert.swift
Lines 13 to 22 in ee01462
/// How to choose `assert` vs. `precondition`: | |
/// - Wherever possible, it is preferable to emit a diagnostic instead of | |
/// using `precondition`. This way the parser won't crash if the condition is | |
/// violated. | |
/// - If you think the diagnostic added above should never be emitted, it is | |
/// fine to also emit an `assertionFailure` in debug builds to make it easier | |
/// to debug the unexpected diagnostic. | |
/// - If in doubt always use `precondition` | |
/// - `assert` should only be used if checking the assertion has a non-trivial | |
/// cost and provides little benefit in terms of safety in release builds. |
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 not really sure it worth, although it trivial, The branch is a result of the subject expression, where the experimentalFeatures
is handled, which is close enough for not checking seriously. Also even if somehow we enter this branch without oldOwnershipOperatorSpellings
feature in release builds, we still have atContextualExpressionModifier()
condtion check below so the check is just to make sure the condition check in ExpressionModifierKeyword.init(lexeme:)
is working.
FWIW, looking at the existing code, do
expression uses precodition
but then
statement uses assert
.
_borrow and _move should be parsed as an ownership operator only when a feature flag is enabled.