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

Initial implementation of or-pattern usefulness checking #66612

Merged
merged 6 commits into from
Dec 1, 2019

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Nov 21, 2019

The title says it all.
I'd like to request a perf run on that, hopefully this doesn't kill performance too much.

cc #54883

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2019
@Centril
Copy link
Contributor

Centril commented Nov 21, 2019

r? @varkor cc @arielb1

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors

This comment has been minimized.

src/test/compile-fail/or-patterns.rs Outdated Show resolved Hide resolved
src/test/compile-fail/or-patterns.rs Outdated Show resolved Hide resolved
src/test/compile-fail/or-patterns.rs Outdated Show resolved Hide resolved
@Centril Centril added the F-or_patterns `#![feature(or_patterns)]` label Nov 21, 2019
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2019
@Nadrieril

This comment has been minimized.

@varkor

This comment has been minimized.

@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 22, 2019

⌛ Trying commit 0030a77 with merge 320df38...

bors added a commit that referenced this pull request Nov 22, 2019
Initial implementation of or-pattern usefulness checking

The title says it all.
I'd like to request a perf run on that, hopefully this doesn't kill performance too much.

cc #54883
@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 23, 2019

☀️ Try build successful - checks-azure
Build commit: 320df38 (320df38ba09a0ebc7c7861ef004719810c60ae1f)

@rust-timer
Copy link
Collaborator

Queued 320df38 with parent 5fa0af2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 320df38, comparison URL.

@Nadrieril Nadrieril requested a review from Centril November 29, 2019 17:24
}

match (0u8,) {
(1 | 1,) => {} // redundancy not detected for now
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you have plans for this somehow... =P

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 do ^^. This is actually necessary if we want to remove the hack for top-level or-patterns and keep the same diagnostics. I'll have a follow-up PR up soon

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Looks good as an initial implementation but then again the algorithm in _match.rs is not something I'm really familiar with so @varkor should do the approving.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

Changes look good — r=me with the nits fixed.

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved
src/test/ui/or-patterns/exhaustiveness-pass.rs Outdated Show resolved Hide resolved
Co-Authored-By: varkor <github@varkor.com>
@varkor
Copy link
Member

varkor commented Nov 30, 2019

Great, thank you very much for working on this!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 30, 2019

📌 Commit 0f4c5fb has been approved by varkor

@bors
Copy link
Contributor

bors commented Nov 30, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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 Nov 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 30, 2019
…rkor

Initial implementation of or-pattern usefulness checking

The title says it all.
I'd like to request a perf run on that, hopefully this doesn't kill performance too much.

cc rust-lang#54883
bors added a commit that referenced this pull request Dec 1, 2019
Rollup of 9 pull requests

Successful merges:

 - #66612 (Initial implementation of or-pattern usefulness checking)
 - #66705 (Atomic as_mut_ptr)
 - #66759 (impl TrustedLen for vec::Drain)
 - #66858 (Use LLVMAddAnalysisPasses instead of Rust's wrapper)
 - #66870 (SimplifyArmIdentity only for locals with the same type)
 - #66883 (rustc_typeck: gate AnonConst's generics on feature(const_generics).)
 - #66889 (Make python-generated source files compatible with rustfmt)
 - #66894 (Remove unneeded prelude imports in libcore tests)
 - #66895 (Feature gating *declarations* => new crate `rustc_feature`)

Failed merges:

 - #66905 (rustc_plugin: Remove some remaining plugin features)

r? @ghost
@bors bors merged commit 0f4c5fb into rust-lang:master Dec 1, 2019
@bors
Copy link
Contributor

bors commented Dec 1, 2019

☔ The latest upstream changes (presumably #66908) 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 Dec 1, 2019
@Nadrieril
Copy link
Member Author

I'm confused, bors merged the PR and then detected a conflict with itself ? Is that a false positive from bors or do I need to rebase on master ?

@Centril
Copy link
Contributor

Centril commented Dec 1, 2019

That's a false positive from the buggy bors; no need to do anything.

@Nadrieril Nadrieril deleted the or-patterns-initial branch December 2, 2019 15:32
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
Remove hack for top-level or-patterns in match checking

Follow-up to rust-lang#66612.

Or-patterns are now truly first-class in match checking. As a side-effect, redundant subpatterns are linted as such, making the `unreachable_patterns` lint a bit more general.

cc rust-lang#54883

r? @varkor
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
Remove hack for top-level or-patterns in match checking

Follow-up to rust-lang#66612.

Or-patterns are now truly first-class in match checking. As a side-effect, redundant subpatterns are linted as such, making the `unreachable_patterns` lint a bit more general.

cc rust-lang#54883

r? @varkor
@mark-i-m
Copy link
Member

mark-i-m commented Dec 5, 2019

the buggy bors

😱 How dare you insult the @bors 💥 HE SEES ALLLLLLLLLL

cc rust-lang/rfcs#2671

@Nadrieril Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns F-or_patterns `#![feature(or_patterns)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants