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

Dogfood {exclusive,half-open} ranges in compiler (nfc) #78244

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

workingjubilee
Copy link
Member

In particular, this allows us to write more explicit matches that
avoid the pitfalls of using a fully general fall-through case, yet
remain fairly ergonomic. Less logic is in guard cases, more is in
the actual exhaustive case analysis.

No functional changes.

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 Oct 22, 2020
@@ -251,7 +251,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
}
ty::Array(ty, len) => match len.try_eval_usize(cx.tcx, cx.param_env) {
// If the array is definitely non-empty, we can do `#[must_use]` checking.
Some(n) if n != 0 => {
Some(n @ 1..) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is quite cute, but I'm not sure it's more readable. I'd like to hear what others think.

Copy link
Member Author

Choose a reason for hiding this comment

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

My overall hypothesis was that this pattern is neither significantly more or less legible to humans, but significantly more legible to the compiler, which allows it to better reason around and assist the programmer, so in the spirit of meeting the machine halfway...

I will note my initial inclination was to move the simple "negative" ( Some(0) | None ) condition first, which might make more sense. Happy to hear what others think, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think I would prefer to exhaustively match by starting with Some(0) | None => false, Some(n).

It might still make sense to use Some(n @ 1..) as a quick reminder to the programmer, but in general I don't have a strong opinion here

@@ -251,7 +251,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedResults {
}
ty::Array(ty, len) => match len.try_eval_usize(cx.tcx, cx.param_env) {
// If the array is definitely non-empty, we can do `#[must_use]` checking.
Some(n) if n != 0 => {
Some(n @ 1..) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think I would prefer to exhaustively match by starting with Some(0) | None => false, Some(n).

It might still make sense to use Some(n @ 1..) as a quick reminder to the programmer, but in general I don't have a strong opinion here

@jyn514 jyn514 added F-exclusive_range_pattern `#![feature(exclusive_range_pattern)]` F-half_open_range_patterns `#![feature(half_open_range_patterns)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 23, 2020
@workingjubilee
Copy link
Member Author

Updated to put the Some(0) | None first.
These are at least appreciated by some, even if it's still subject to debate, see:
#37854 (comment)
Also there is a motion to stabilize this specific pattern, which is a faster result than I expected. :^)
#67264 (comment)

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.

I think after this one comment, and squashing the commits, I'm happy.

compiler/rustc_lint/src/unused.rs Outdated Show resolved Hide resolved
In particular, this allows us to write more explicit matches that
avoid the pitfalls of using a fully general fall-through case, yet
remain fairly ergonomic. Less logic is in guard cases, more is in
the actual exhaustive case analysis.

No functional changes.
@varkor
Copy link
Member

varkor commented Oct 29, 2020

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 29, 2020

📌 Commit 0e88db7 has been approved by varkor

@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 Oct 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2020
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#75078 (Improve documentation for slice strip_* functions)
 - rust-lang#76138 (Explain fully qualified syntax for `Rc` and `Arc`)
 - rust-lang#78244 (Dogfood {exclusive,half-open} ranges in compiler (nfc))
 - rust-lang#78422 (Do not ICE on invalid input)
 - rust-lang#78423 (rustc_span: improve bounds checks in byte_pos_to_line_and_col)
 - rust-lang#78431 (Prefer new associated numeric consts in float error messages)
 - rust-lang#78462 (Use unwrapDIPtr because the Scope may be null.)
 - rust-lang#78493 (Update cargo)
 - rust-lang#78499 (Prevent String::retain from creating non-utf8 strings when abusing panic)
 - rust-lang#78505 (Update Clippy - temporary_cstring_as_ptr deprecation)
 - rust-lang#78527 (Fix some more typos)

Failed merges:

r? `@ghost`
@bors bors merged commit 9867e54 into rust-lang:master Oct 30, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 30, 2020
@workingjubilee workingjubilee deleted the dogfood-fancy-ranges branch April 23, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-exclusive_range_pattern `#![feature(exclusive_range_pattern)]` F-half_open_range_patterns `#![feature(half_open_range_patterns)]` 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
Development

Successfully merging this pull request may close these issues.

7 participants