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.

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

Even more lexer improvements #102508

Merged
merged 8 commits into from
Oct 3, 2022
Merged

Even more lexer improvements #102508

merged 8 commits into from
Oct 3, 2022

Conversation

nnethercote
Copy link
Contributor

These are just about code clarity, rather than performance.

r? @matklad

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 30, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2022
matklad
matklad reviewed Sep 30, 2022
View reviewed changes
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM, but perhaps this needs a comment on the semanics of spacning for things which are not ops

@matklad
Copy link
Member

matklad commented Sep 30, 2022

@bors delegate+

@bors
Copy link
Collaborator

bors commented Sep 30, 2022

✌️ @nnethercote can now approve this pull request

@nnethercote nnethercote force-pushed the even-more-lexer-improvements branch from 7128590 to 46907fb Compare September 30, 2022 22:00
@nnethercote
Copy link
Contributor Author

@bors r=matklad

@bors
Copy link
Collaborator

bors commented Sep 30, 2022

📌 Commit 46907fb53c4a18deab627bb8f8ac889fb47843c1 has been approved by matklad

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 Sep 30, 2022
@bors
Copy link
Collaborator

bors commented Oct 1, 2022

☔ The latest upstream changes (presumably #102526) made this pull request unmergeable. Please resolve the merge conflicts.

@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 Oct 1, 2022
@nnethercote
Clarify operator splitting.
3be86e6 
I found this code hard to read.
@nnethercote
Add comments to Spacing.
bbb53bf
@nnethercote
Rewrite Token::is_op.
40e4827 
An exhaustive match is more readable and more future-proof.
@nnethercote
Add comments to TokenCursor::desugar.
9de9cf1 
It took me some time to work out what this code was doing.
@nnethercote
Merge parse_token_trees_until_close_delim and parse_all_token_trees.
ce76768 
Because they're very similar, and this will allow some follow-up
changes.
@nnethercote
Inline and remove parse_token_tree_non_delim_non_eof.
8d0754d 
It has a single call site.
@nnethercote
Remove TokenStreamBuilder.
a822d08 
It's now only used in one function. Also, the "should we glue the
tokens?" check is only necessary when pushing a `TokenTree::Token`, not
when pushing a `TokenTree::Delimited`.

As part of this, we now do the "should we glue the tokens?" check
immediately, which avoids having look back at the previous token. It
also puts all the logic dealing with token gluing in a single place.
@nnethercote
Invert is_top_level to avoid negation.
4e5ddf1
@nnethercote nnethercote force-pushed the even-more-lexer-improvements branch from 46907fb to 4e5ddf1 Compare October 3, 2022 01:40
@nnethercote
Copy link
Contributor Author

I rebased.

@bors r=matklad

@bors
Copy link
Collaborator

bors commented Oct 3, 2022

📌 Commit 4e5ddf1 has been approved by matklad

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2022
@bors
Copy link
Collaborator

bors commented Oct 3, 2022

⌛ Testing commit 4e5ddf1 with merge dbaf3e6...

@bors
Copy link
Collaborator

bors commented Oct 3, 2022

☀️ Test successful - checks-actions
Approved by: matklad
Pushing dbaf3e6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2022
@bors bors merged commit dbaf3e6 into rust-lang:master Oct 3, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 3, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dbaf3e6): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 -2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)		 -4.4% [-4.8%, -4.1%] 3
All ❌✅ (primary) -2.4% [-2.4%, -2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@nnethercote nnethercote deleted the even-more-lexer-improvements branch October 3, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

Assignees

@matklad matklad

Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @matklad @bors @rust-timer @rust-highfive @rustbot