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

Compare empty blocks for equality based on tokens #6843

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 4, 2021

fixes: #1390

This only considers empty blocks for now, though we should also catch something like this:

match 0 {
    0 => {
        do_something();
        trace!(0);
        0
    }
    1 => {
        do_something();
        trace!(1);
        1
    }
    x => x,
}

As far as I can tell there aren't any negative effects on other lints. These blocks only happen to be the same for a given compilation, not all compilations.

changelog: Fix match_on_same_arms and others. Only consider empty blocks equal if the tokens contained are the same.

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 4, 2021
@Manishearth
Copy link
Member

Hmmm. This seems like it would be a large perf impact from reparsing, no? cc @flip1995

Can we potentially have a tokenize boolean argument that does equality checking based on tokens, and first do a regular equality check and only then do a token based one? This also means this function stays useful for other use cases.

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 4, 2021

It's only running the lexer, not the full parser. It will also only run once a given block is determined to be empty. The performance impact should be basically zero.

@Manishearth
Copy link
Member

Oh, only for empty blocks, missed that sorry

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2021

Why tokenize at all and not just compare the snippets directly? (You probably will have to trim away the braces and all white spaces and line breaks before comparing though)

This would also take care of comments, that are not identical inside the blocks.

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 4, 2021

The tokenizing is to deal with the whitespace. e.g. x y isn't the same as xy, but ( x ) is the same as (x).

Should probably not skip comments though.

@Manishearth
Copy link
Member

@bors r+

I think it's fine to not skip comments

@bors
Copy link
Collaborator

bors commented Mar 4, 2021

📌 Commit 39c5e86 has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Mar 4, 2021

⌛ Testing commit 39c5e86 with merge 7be3a32...

@bors
Copy link
Collaborator

bors commented Mar 4, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 7be3a32 to master...

@bors bors merged commit 7be3a32 into rust-lang:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive with match_on_same_arms and macros
5 participants