-
Notifications
You must be signed in to change notification settings - Fork 10.5k
AST: Remove LiteralExpr::shallowClone() #26118
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
AST: Remove LiteralExpr::shallowClone() #26118
Conversation
@swift-ci Please smoke test |
@@ -6685,26 +6685,9 @@ Expr *ExprRewriter::convertLiteralInPlace(Expr *literal, | |||
DeclName builtinLiteralFuncName, | |||
Diag<> brokenProtocolDiag, | |||
Diag<> brokenBuiltinProtocolDiag) { | |||
auto &tc = cs.getTypeChecker(); |
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.
Too much deleting here.
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.
Yep :)
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.
... or not enough deleting further down! See updated version :)
6c6fd3c
to
866cd4d
Compare
@swift-ci Please smoke test |
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.
Seems reasonable to me.
At one point it was used to work around problems with re-type checking literals in CSApply, but all of our tests appear to pass with this removed.
866cd4d
to
5022d8b
Compare
@swift-ci Please smoke test |
@@ -2,5 +2,5 @@ | |||
|
|||
func test(_ x: Int) -> Int { | |||
return x + nil | |||
// expected-error@-1 {{cannot convert value of type 'Int' to expected argument type '_.Stride'}} | |||
// expected-error@-1 {{type of expression is ambiguous without more context}} |
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.
@xedin I noticed this diagnostic did change. Are you OK with this? The test case was originally added when a crash was fixed. I don't think the old diagnostic actually gave you any useful information (the real problem is there's no suitable overload of +
-- as far as I can see the appearance of Stride in the old message is an emergent behavior of CSDiag and CalleeCandidateDiagnostics)
At one point it was used to work around problems with re-type checking
literals in CSApply, but all of our tests appear to pass with this removed.