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

get rid of tidy 'unnecessarily ignored' warnings #98717

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 30, 2022

I think these warnings are quite pointless: when I say allow(foo) in my code, that doesn't necessarily mean that I expect foo to happen -- it just means that I am okay with foo happening.

For example, having to add and remove ignore-tidy-linelength as the longest line in the file keeps growing and shrinking is just annoying and doesn't benefit anyone, IMO. This usually incurs two CI roundtrips: first CI tells you that line lengths in your test file are ignored unnecessarily, so you go and remove that attribute; then CI tells you that now your line numbers changed, so you re-bless your tests (often takes >5min if parts of rustc need rebuilding because ./x.py fmt changed something somewhere). That's just a lot of wasted effort and time and patience.

@rust-highfive
Copy link
Collaborator

r? @jyn514

(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 Jun 30, 2022
@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jun 30, 2022
@jyn514
Copy link
Member

jyn514 commented Jun 30, 2022

I agree warning about line length is unhelpful and just leads to unnecessary CI failures. I am less sure about the others - I think having // ignore-crlf in a file without CRLF line endings is quite misleading, and will leave to cruft in the repo over time as the warnings are added. Can you add back all the warnings except for ignore-linelength and ignore-filelength?

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 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 Jun 30, 2022
@RalfJung
Copy link
Member Author

Can you add back all the warnings except for ignore-linelength and ignore-filelength?

Fair, I have not run into the others.

@jyn514
Copy link
Member

jyn514 commented Jun 30, 2022

r=me with CI passing

@jyn514 jyn514 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 Jun 30, 2022
@Dylan-DPC
Copy link
Member

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Jun 30, 2022

📌 Commit 8515475 has been approved by jyn514

@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 Jun 30, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 30, 2022
…=jyn514

get rid of tidy 'unnecessarily ignored' warnings

I think these warnings are quite pointless: when I say `allow(foo)` in my code, that doesn't necessarily mean that I expect `foo` to happen -- it just means that I am okay with `foo` happening.

For example, having to add and remove `ignore-tidy-linelength` as the longest line in the file keeps growing and shrinking is just annoying and doesn't benefit anyone, IMO. This usually incurs *two* CI roundtrips: first CI tells you that line lengths in your test file are ignored unnecessarily, so you go and remove that attribute; then CI tells you that now your line numbers changed, so you re-bless your tests (often takes >5min if parts of rustc need rebuilding because `./x.py fmt` changed something somewhere). That's just a lot of wasted effort and time and patience.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 30, 2022
…=jyn514

get rid of tidy 'unnecessarily ignored' warnings

I think these warnings are quite pointless: when I say `allow(foo)` in my code, that doesn't necessarily mean that I expect `foo` to happen -- it just means that I am okay with `foo` happening.

For example, having to add and remove `ignore-tidy-linelength` as the longest line in the file keeps growing and shrinking is just annoying and doesn't benefit anyone, IMO. This usually incurs *two* CI roundtrips: first CI tells you that line lengths in your test file are ignored unnecessarily, so you go and remove that attribute; then CI tells you that now your line numbers changed, so you re-bless your tests (often takes >5min if parts of rustc need rebuilding because `./x.py fmt` changed something somewhere). That's just a lot of wasted effort and time and patience.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#97629 ([core] add `Exclusive` to sync)
 - rust-lang#98503 (fix data race in thread::scope)
 - rust-lang#98670 (llvm-wrapper: adapt for LLVMConstExtractValue removal)
 - rust-lang#98671 (Fix source sidebar bugs)
 - rust-lang#98677 (For diagnostic information of Boolean, remind it as use the type: 'bool')
 - rust-lang#98684 (add test for 72793)
 - rust-lang#98688 (interpret: add From<&MplaceTy> for PlaceTy)
 - rust-lang#98695 (use "or pattern")
 - rust-lang#98709 (Remove unneeded methods declaration for old web browsers)
 - rust-lang#98717 (get rid of tidy 'unnecessarily ignored' warnings)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6e918b4 into rust-lang:master Jul 1, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 1, 2022
@RalfJung RalfJung deleted the make-tidy-less-annoying branch July 2, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants