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

Remove trivia tokens #76170

Merged
merged 3 commits into from
Sep 2, 2020
Merged

Remove trivia tokens #76170

merged 3 commits into from
Sep 2, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Aug 31, 2020

r? @ghost

@matklad
Copy link
Member Author

matklad commented Aug 31, 2020

Builds on the #76166

This doesn't actually remove associated enum variants just yet, but, surprisingly, just never constructiong those variants works without any additional tweaks anywhere.

cc @petrochenkov

@matklad
Copy link
Member Author

matklad commented Aug 31, 2020

I think the end result here would be something like

crate fn tokenize_into_token_trees(
    sess: &'a ParseSess,
    source_file: Lrc<rustc_span::SourceFile>,
    override_span: Option<Span>,
) -> (PResult<'a, TokenStream>, Vec<UnmatchedBrace>)

Ie, StringReader and TokenTreesReader would become, at minimum, private, but may actually get completely dissolved

@petrochenkov petrochenkov self-assigned this Aug 31, 2020
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2020
@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2020
@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-parselib labels Sep 1, 2020
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 1, 2020
@matklad matklad changed the title WIP: Remove trivia tokens Remove trivia tokens Sep 1, 2020
@matklad matklad 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 Sep 1, 2020
@matklad
Copy link
Member Author

matklad commented Sep 1, 2020

Ready for review @petrochenkov!

I've decided to leave overall code shape changes (fully privatising StringReader) to a follow up PR, to separate pure refactoring from changes to core data structures.

@matklad
Copy link
Member Author

matklad commented Sep 1, 2020

Wait, tests fail locally...

@matklad matklad 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 Sep 1, 2020
@matklad
Simplify TokenTreesReader
fabd8a6 
This `joint_to_prev` bit of state is no longer needed.
matklad
matklad commented Sep 1, 2020
View reviewed changes
compiler/rustc_parse/src/lexer/tokentrees.rs
Comment on lines +266 to +268
if !self.token.is_op() {
is_joint = NonJoint;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I originally forgot this condition, and that failed some proc-macro tests. This is a bit weird -- I would expect this jointness censoring to happen at the proc_macro_srv layer, and not here.

Copy link
Contributor

@petrochenkov petrochenkov Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this check should be moved to proc macro server.
Also lexer should produce the jointness flag for the "delimited group" token trees as well (the match branches above). (Two of such flags, actually, for both the opening and the closing delimiter.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I do this in this PR, or a separate one?

I've also rebased #75528 on top of this PR, seems to work

matklad@21bf6ce

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I do this in this PR, or a separate one?

Whichever is more convenient.

@matklad matklad 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 Sep 1, 2020
@matklad
Copy link
Member Author

matklad commented Sep 1, 2020

Ok, should be good to go now!

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2020

📌 Commit fabd8a6 has been approved by petrochenkov

@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 1, 2020
@bors
Copy link
Contributor

bors commented Sep 2, 2020

⌛ Testing commit fabd8a6 with merge b4acb11...

@bors
Copy link
Contributor

bors commented Sep 2, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing b4acb11 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2020
@bors bors merged commit b4acb11 into rust-lang:master Sep 2, 2020
@matklad matklad deleted the notrivia branch September 2, 2020 07:32
@petrochenkov petrochenkov mentioned this pull request Dec 11, 2020
Closed
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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.48.0
Development

Successfully merging this pull request may close these issues.
5 participants
@matklad @petrochenkov @bors @cuviper @jyn514