Skip to content
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

Improve parser #125117

Merged
merged 4 commits into from
May 18, 2024
Merged

Improve parser #125117

merged 4 commits into from
May 18, 2024

Conversation

dev-ardi
Copy link
Contributor

@dev-ardi dev-ardi commented May 14, 2024

  • Add a few more help diagnostics to incorrect semicolons
  • Overall improved that function
  • Addded a few comments
  • Renamed diff_marker fns to git_diff_marker

@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 14, 2024
@wesleywiser
Copy link
Member

Nice improvements!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 14, 2024

📌 Commit bee186b has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2024
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
(0..3).all(|i| self.look_ahead(i, |tok| tok == long_kind))
&& self.look_ahead(3, |tok| tok == short_kind)
}

fn diff_marker(&mut self, long_kind: &TokenKind, short_kind: &TokenKind) -> Option<Span> {
if self.is_diff_marker(long_kind, short_kind) {
fn git_diff_marker(&mut self, long_kind: &TokenKind, short_kind: &TokenKind) -> Option<Span> {
Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly it was intentional to not mention git here. I'd need to look for the original PRs. CC @estebank

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, diff markers are used by git, but not exclusive to it.

@fmease
Copy link
Member

fmease commented May 14, 2024

Sorry but the markdown docs are butchered.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 14, 2024
@dev-ardi
Copy link
Contributor Author

Thanks for noticing!

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2024
@fmease
Copy link
Member

fmease commented May 14, 2024

Thanks. One last thing, could you squash the commits into one?

@dev-ardi
Copy link
Contributor Author

All of them or just the "applied suggestions" one? If all with what name?

@fmease
Copy link
Member

fmease commented May 14, 2024

Eh, I guess git-fixup the “apply suggs” commit into 9db789a should be fine

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2024
@dev-ardi
Copy link
Contributor Author

@rustbot ready
About the git marker, what would be the reason not to include it in the name?

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2024
@fmease
Copy link
Member

fmease commented May 15, 2024

About the git marker, what would be the reason not to include it in the name?

As far as I'm aware, conflict markers (the >>>, ===, |||, <<<) are not specific to git but I'm sure about the specifics. Other VCSs also use them like SVN and Mercurial. I could be wrong but GNU's diff3 (three-way "merges") also produce them, not sure about GNU's diff (both from diffutils).

I think now might be the best time to rename [git] diff marker to conflict markers which is the more appropriate term as touched upon in #113826. If you could do that in this PR, that would be great!

Diff markers are +++, ---, @@, +, - and they come from the patch file format.

@dev-ardi
Copy link
Contributor Author

Maybe vcs_conflict_marker then?

@fmease fmease self-assigned this May 15, 2024
@fmease
Copy link
Member

fmease commented May 15, 2024

Maybe vcs_conflict_marker then?

I guess that's fine, yes. From a purist / theoretical point of view, these conflict markers not necessarily tied to VCSs but to be fair, in practice, that shouldn't matter. CC https://www.gnu.org/software/diffutils/, https://www.geeksforgeeks.org/diff3-command-in-linux-with-examples/ (^^')

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2024
@dev-ardi
Copy link
Contributor Author

dev-ardi commented May 17, 2024

@rustbot ready
Ended up going for vcs_conflict_marker for all pub methods to make sure that whoever is reading that knows outright what it's referring to.

Question about the review: I rebased edit the specific commit with the fix. Should I have made the fix commit and made the fixup rebase after you approve so that it's easier to review?

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Those a very nice changes, thanks a lot for your patience! I've noticed two small things but they're not worth blocking on. That can be done when fixing #113826, maybe you're up for it? ;).

Comment on lines +2959 to +2962
/// * `>>>>>`
/// * `=====`
/// * `<<<<<`
///
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// * `>>>>>`
/// * `=====`
/// * `<<<<<`
///
/// * `>>>>>`
/// * `|||||`
/// * `=====`
/// * `<<<<<`

@@ -567,7 +567,7 @@ impl<'a> Parser<'a> {
if self.token == token::Eof {
break;
}
if self.is_diff_marker(&TokenKind::BinOp(token::Shl), &TokenKind::Lt) {
if self.is_vcs_conflict_marker(&TokenKind::BinOp(token::Shl), &TokenKind::Lt) {
// Account for `<<<<<<<` diff markers. We can't proactively error here because
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Account for `<<<<<<<` diff markers. We can't proactively error here because
// Account for `<<<<<<<` conflict markers. We can't proactively error here because

@fmease
Copy link
Member

fmease commented May 18, 2024

@bors r=wesleywiser,fmease rollup

@bors
Copy link
Contributor

bors commented May 18, 2024

📌 Commit f8433a8 has been approved by wesleywiser,fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#125117 (Improve parser)
 - rust-lang#125184 (Fix ICE in non-operand `aggregate_raw_ptr` intrinsic codegen)
 - rust-lang#125240 (Temporarily revert to NonZeroUsize in rustc-abi to fix building on stable)
 - rust-lang#125248 (Migrate `run-make/rustdoc-scrape-examples-invalid-expr` to `rmake.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@fmease

This comment was marked as outdated.

@bors bors merged commit f9bf759 into rust-lang:master May 18, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2024
Rollup merge of rust-lang#125117 - dev-ardi:improve-parser, r=wesleywiser,fmease

Improve parser

Fixes rust-lang#124935.

- Add a few more help diagnostics to incorrect semicolons
- Overall improved that function
- Addded a few comments
- Renamed diff_marker fns to git_diff_marker
@dev-ardi dev-ardi deleted the improve-parser branch May 19, 2024 12:32
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 21, 2024
Fix parsing of erroneously placed semicolons

This closes rust-lang#124935, is a continuation of rust-lang#125245 after rebasing rust-lang#125117.

Thanks `@gurry` for your code and sorry for making it confusing :P

r? fmease
fmease added a commit to fmease/rust that referenced this pull request May 21, 2024
Fix parsing of erroneously placed semicolons

This closes rust-lang#124935, is a continuation of rust-lang#125245 after rebasing rust-lang#125117.

Thanks ``@gurry`` for your code and sorry for making it confusing :P

r? fmease
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 21, 2024
Fix parsing of erroneously placed semicolons

This closes rust-lang#124935, is a continuation of rust-lang#125245 after rebasing rust-lang#125117.

Thanks ```@gurry``` for your code and sorry for making it confusing :P

r? fmease
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2024
Rollup merge of rust-lang#125276 - dev-ardi:no-main-diag, r=fmease

Fix parsing of erroneously placed semicolons

This closes rust-lang#124935, is a continuation of rust-lang#125245 after rebasing rust-lang#125117.

Thanks ```@gurry``` for your code and sorry for making it confusing :P

r? fmease
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants