Skip to content

Renamed isAt* funcs to at* in Parser and Parser.Lookahead #1996

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 5, 2023
Merged

Renamed isAt* funcs to at* in Parser and Parser.Lookahead #1996

merged 1 commit into from
Aug 5, 2023

Conversation

@natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Aug 3, 2023 •
edited
Loading

Motivation

Closes #1976. This pull request is a step towards making the Parser API consistent, and renaming some of the boolean isAt* functions with at*.

Changes

  • Lexer.Cursor.isAtStringInterpolationAnchor — private, so I'll rename it and change any spots where it's invoked. Deprecating the existing function should not be necessary.
  • Lexer.Cursor.isAtEscapedNewline — same as above.
  • Parser.Lookahead has two fileprivate functions isAtStartOfPostfixExprSuffix and isInBindingPatternPosition — I think it's best to rename both of them so the code in that particular extension is consistent. It would be weird to have atStartOfPostfixExprSuffix and isIn*.
  • Parser.isAtPythonStyleInheritanceClause
  • In SwiftParser/Statements.swift we have Parser.Lookahead extension with isAtStartOfSwitchCase, isStartOfStatement, isStartOfConditionalSwitchCases.
    • These are not marked as private, so I think we should keep the original version and deprecate it.
    • Should I rename isStartOf* as well for consistency?
  • SwiftParser/Types.swift has Parser.Lookahead.isAtFunctionTypeArrow — also not marked as private.

TODO

  • Decide if we need deprecated versions for non-private funcs
  • Tests pass
  • Formatting OK

@natikgadzhi natikgadzhi requested a review from ahoppen as a code owner August 3, 2023 15:59
ahoppen
ahoppen reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you.

Decide if we need deprecated versions for non-private funcs

These functions aren’t public, so there’s no need for deprecation.

Sources/SwiftParser/Expressions.swift
// Helper function to see if we can parse member reference like suffixes
// inside '#if'.
fileprivate mutating func isAtStartOfPostfixExprSuffix() -> Bool {
fileprivate mutating func atStartOfPostfixExprSuffix() -> Bool {
Copy link
Member

@ahoppen ahoppen Aug 3, 2023

Choose a reason for hiding this comment

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

I would keep the functions in Lexer.Cursor in the isAt style. The main checking function of Lexer.Cursor also is is(offset: Int = 0, at: CharcterByte), so that fits with the isAt style. The parser just uses at(_: TokenSpec).

Copy link
Contributor Author

@natikgadzhi natikgadzhi Aug 3, 2023

Choose a reason for hiding this comment

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

Got it! One sec, I'll clean this up.

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 3, 2023

@ahoppen, thank you for reviewing this! Cleaned up Lexer, so I think it should be good to go now.

@natikgadzhi natikgadzhi requested a review from ahoppen August 3, 2023 17:27
ahoppen
ahoppen reviewed Aug 3, 2023
View reviewed changes
Sources/SwiftParser/Lexer/Cursor.swift Outdated
/// Returns `true` if the cursor is positioned at `\##(` with `delimiterLength`
/// pound characters.
private func isAtStringInterpolationAnchor(delimiterLength: Int) -> Bool {
private func atStringInterpolationAnchor(delimiterLength: Int) -> Bool {
Copy link
Member

@ahoppen ahoppen Aug 3, 2023

Choose a reason for hiding this comment

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

Oh, I put the previous comment on the wrong line. I meant that we should keep the functions in Cursor.swift. Everything else should be renamed. I’m sorry.

Copy link
Contributor Author

@natikgadzhi natikgadzhi Aug 3, 2023

Choose a reason for hiding this comment

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

Ah, mb, I should've read it more carefully too. Okay, so reverting this change, but renaming SwiftParser/Expressions.swift.

ahoppen
ahoppen approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looking good 👍🏽

@ahoppen
Copy link
Member

ahoppen commented Aug 3, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge August 3, 2023 21:36
auto-merge was automatically disabled August 3, 2023 22:05

Head branch was pushed to by a user without write access

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 3, 2023

@ahoppen cleaned it up. Parser and Parser.Lookahead use at* notation. Cursor is back to isAt* notation.

@ahoppen ahoppen enabled auto-merge August 4, 2023 00:01
@ahoppen ahoppen disabled auto-merge August 4, 2023 00:01
@ahoppen ahoppen enabled auto-merge (squash) August 4, 2023 00:01
@ahoppen
Copy link
Member

ahoppen commented Aug 4, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Aug 4, 2023

@swift-ci Please test Windows

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 4, 2023

@ahoppen I see the formatting issues on the CI output, and I can clean them up. I'll build swift-format from main as you advised and work through them.

@ahoppen
Copy link
Member

ahoppen commented Aug 4, 2023

Great. Thank you for fixing the formatting issue. Also: The macOS CI failure is unrelated to your changes.

auto-merge was automatically disabled August 4, 2023 22:40

Head branch was pushed to by a user without write access

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 4, 2023

My local swift-format is still building, but I think I've fixed the only problem it found. Rebased over main to see if the macOS test is fixed there.

@ahoppen
Copy link
Member

ahoppen commented Aug 4, 2023

OK, let’s try again.

Also, just a note: We prefer to squash the commits of a PR because it just creates a nicer git history. I’ll put together some documentation with reasons for that in the upcoming days.

@swift-ci Please test

@natikgadzhi natikgadzhi mentioned this pull request Aug 4, 2023
Merged
@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 4, 2023 •
edited
Loading

@ahoppen, agreed re: squashing — I usually end up doing that on the GitHub side when merging, but I'm happy to git rebase -i and squash so that after a review there's a single commit per pull request!

I usually keep commits separate to retain the step by step history of a PR while we're still talking through it, but once the review concludes, I can absolutely squash them.

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 4, 2023

There's Parser.isContextualExpressionModifier — should we rename it as well for consistency? @ahoppen

@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge August 5, 2023 03:48
@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

There's Parser.isContextualExpressionModifier — should we rename it as well for consistency? @ahoppen

Yes, I think that would be a good idea as well.

auto-merge was automatically disabled August 5, 2023 04:17

Head branch was pushed to by a user without write access

@natikgadzhi
Copy link
Contributor Author

natikgadzhi commented Aug 5, 2023

Cleaned it up, and squashed into a single commit.

@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge August 5, 2023 04:40
@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 861333c into swiftlang:main Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rename isAt* functions in SwiftParser to at*

2 participants

@natikgadzhi @ahoppen