Skip to content

Merge nominal parsing into the one function #968

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
Oct 21, 2022

Conversation

bnbarham
Copy link
Contributor

All nominals had extremely similar parse functions. Merge them into the one function so that improvements to one improves them all.

@bnbarham bnbarham requested a review from CodaFi October 17, 2022 17:17
@bnbarham bnbarham requested a review from ahoppen as a code owner October 17, 2022 17:17
Copy link
Member

@DougGregor DougGregor 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! This was bugging me

@bnbarham
Copy link
Contributor Author

Thank you! This was bugging me

Heh, I actually did this because I noticed that actor didn't have the same identifier recovery as the rest. That didn't pan out, though at least if we change the lookahead in the future we'll get it for free now.

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@@ -804,7 +804,7 @@ final class DeclarationTests: XCTestCase {

func testDontRecoverFromDeclKeyword() {
AssertParse(
"func foo(first second 1️⃣third 3️⃣struct4️⃣: Int) {}",
"func foo(first second 1️⃣third 2️⃣struct3️⃣: Int4️⃣) {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously parsing just struct with all unexpected, but now (after also checking for start of line) parses the remained as a struct instead.

) -> T where T: NominalTypeDeclarationTrait {
let (unexpectedBeforeIntroducerKeyword, introducerKeyword) = self.eat(introucerHandle)
let (unexpectedBeforeName, name) = self.expectIdentifier(keywordRecovery: true)
if unexpectedBeforeName == nil && name.isMissing && self.currentToken.isAtStartOfLine {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And to be clear - self.currentToken.isAtStartOfLine is the change here.

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the unify-nominals branch 2 times, most recently from 4f4583e to 0030cac Compare October 21, 2022 00:24
@bnbarham
Copy link
Contributor Author

@swift-ci please test

Comment on lines +185 to +186
DiagnosticSpec(locationMarker: "3️⃣", message: "expected identifier in protocol"),
DiagnosticSpec(locationMarker: "4️⃣", message: "unexpected code '>(x: T)' in protocol"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure this is no worse. But... also isn't great.

All nominals had extremely similar parse functions. Merge them into the
one function so that improvements to one improves them all.
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit 59d7f42 into swiftlang:main Oct 21, 2022
@bnbarham bnbarham deleted the unify-nominals branch October 21, 2022 23:06
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