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

New lint: manual-range-contains #6177

Merged
merged 1 commit into from
Oct 25, 2020
Merged

New lint: manual-range-contains #6177

merged 1 commit into from
Oct 25, 2020

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Oct 15, 2020 •
edited
Loading

This fixes #1110, at least for the contains-suggesting part.

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

changelog: new lint: manual-range-contains

@rust-highfive
Copy link

r? @flip1995

(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 15, 2020
@llogiq
Copy link
Contributor Author

llogiq commented Oct 15, 2020

error seems unrelated, we may need another rustup.

@llogiq llogiq force-pushed the manual-range-contains branch from a77d6ee to db53735 Compare October 15, 2020 21:29
@josephlr
Copy link
Contributor

@llogiq I looks like we need to sync with rustc, and the process currently takes over and hour even after you patch git to make things work better (see #5565). I'm generating the PR right now. I'll ping this issue when the PR is up.

@josephlr
Copy link
Contributor

#6178 is up.

@llogiq
Copy link
Contributor Author

llogiq commented Oct 16, 2020

@josephlr So what do I need to do now? I've updated rust and pulled from master but still get an error.

@ebroto
Copy link
Member

ebroto commented Oct 16, 2020

@llogiq it should work now if you rebase on master (#6184)

@llogiq llogiq force-pushed the manual-range-contains branch from db53735 to a0c2257 Compare October 17, 2020 07:44
@llogiq
Copy link
Contributor Author

llogiq commented Oct 17, 2020

rebased. Thanks @ebroto.

flip1995
flip1995 reviewed Oct 17, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Impl LGTM.

Also, since you will read this comment: You have some stale branches in the rust-lang/rust-clippy repository. We try to delete all branches, that aren't necessary for the infra: #4745 Could you please move them to a fork and then delete them here in this repo?

clippy_lints/src/ranges.rs Outdated Show resolved Hide resolved
tests/ui/range.rs Outdated Show resolved Hide resolved
clippy_lints/src/ranges.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 17, 2020
@llogiq
Copy link
Contributor Author

llogiq commented Oct 17, 2020

I don't think I'll need them anymore. Will delete them once I get to my PC.

@llogiq llogiq force-pushed the manual-range-contains branch from a0c2257 to 122ccbc Compare October 17, 2020 20:29
@llogiq
Copy link
Contributor Author

llogiq commented Oct 17, 2020

@flip1995 Done....and...we need yet another rustup.

@ebroto
Copy link
Member

ebroto commented Oct 17, 2020

Should be fixed by #6189

@llogiq llogiq force-pushed the manual-range-contains branch 2 times, most recently from 084ef72 to 370c8cc Compare October 17, 2020 21:58
@llogiq
Copy link
Contributor Author

llogiq commented Oct 17, 2020

Cool. Now there seems to be yet another unrelated error!?

@llogiq llogiq force-pushed the manual-range-contains branch from 370c8cc to b9953fa Compare October 17, 2020 22:54
@llogiq
Copy link
Contributor Author

llogiq commented Oct 17, 2020

Nevermind, I fixed it en passant. Seems to relate to String::new() now being const fn.

@llogiq
Copy link
Contributor Author

llogiq commented Oct 19, 2020

@flip1995 r?

@llogiq llogiq force-pushed the manual-range-contains branch from b9953fa to 533d767 Compare October 21, 2020 20:33
@llogiq llogiq force-pushed the manual-range-contains branch from 533d767 to c693de3 Compare October 22, 2020 06:46
@llogiq
Copy link
Contributor Author

llogiq commented Oct 22, 2020

Huh? There was some strange interaction (presumably with const stability) in the or_fun_call test. It's fixed now. r? @flip1995

@llogiq
Copy link
Contributor Author

llogiq commented Oct 24, 2020

Since @flip1995 is currently busy, r? @ebroto

@ebroto ebroto 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 from the author. (Use `@rustbot ready` to update this status) labels Oct 25, 2020
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Oct 25, 2020

📌 Commit c693de3 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Oct 25, 2020

⌛ Testing commit c693de3 with merge 90cb25d...

@bors
Copy link
Contributor

bors commented Oct 25, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 90cb25d to master...

@bors bors merged commit 90cb25d into master Oct 25, 2020
@llogiq llogiq deleted the manual-range-contains branch October 26, 2020 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@ebroto ebroto

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint wrong integer range comparisons
6 participants
@llogiq @rust-highfive @josephlr @ebroto @flip1995 @bors