Skip to content

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

Merged
merged 1 commit into from
Jul 13, 2019

Conversation

slavapestov
Copy link
Contributor

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.

@slavapestov slavapestov requested review from xedin and pschuh July 12, 2019 20:01
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@@ -6685,26 +6685,9 @@ Expr *ExprRewriter::convertLiteralInPlace(Expr *literal,
DeclName builtinLiteralFuncName,
Diag<> brokenProtocolDiag,
Diag<> brokenBuiltinProtocolDiag) {
auto &tc = cs.getTypeChecker();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much deleting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :)

Copy link
Contributor Author

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

@slavapestov slavapestov force-pushed the shallow-clone-be-gone branch from 6c6fd3c to 866cd4d Compare July 12, 2019 20:17
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

Copy link
Contributor

@xedin xedin left a 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.
@slavapestov slavapestov force-pushed the shallow-clone-be-gone branch from 866cd4d to 5022d8b Compare July 12, 2019 23:08
@slavapestov
Copy link
Contributor Author

@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}}
Copy link
Contributor Author

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)

@slavapestov slavapestov merged commit 675141b into swiftlang:master Jul 13, 2019
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.

3 participants