Skip to content

/rust

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

code suggestions for unused-mut, while-true, deprecated-attribute, and unused-parens lints #44942

Merged
merged 5 commits into from Oct 2, 2017

Conversation

Reviewers

@estebank estebank

Assignees

@estebank estebank

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zackmdavis @estebank @bors @zachreizner
@zackmdavis
Copy link
Member

zackmdavis commented Sep 30, 2017

lint_suggestions

r? @estebank
@zackmdavis
fix comment typo, `CodeSuggestion` path in doc comment 
`CodeSuggestion` doesn't live in the `diagnostic` module.
d663003

@rust-highfive rust-highfive assigned estebank Sep 30, 2017

@estebank

estebank approved these changes Sep 30, 2017
View changes

Copy link
Contributor

estebank left a comment

Small nitpicks, r=me once you deal with the first two.
src/librustc_lint/builtin.rs Outdated
"denote infinite loops with loop { ... }");
let msg = "denote infinite loops with `loop { ... }`";
let mut err = cx.struct_span_lint(WHILE_TRUE,
e.span, msg);

This comment has been minimized.

Sign in to view
Copy link
@estebank

estebank Sep 30, 2017

Contributor

Fits in one line?
src/librustc_lint/builtin.rs Outdated
e.span, msg);
let condition_span = cx.tcx.sess.codemap().def_span(e.span);
err.span_suggestion_short(condition_span, "use `loop`",
"loop".to_owned());

This comment has been minimized.

Sign in to view
Copy link
@estebank

estebank Sep 30, 2017

Contributor

When all arguments don't fit, I like to keep one line per argument (indented to the first one in the same line as the method).
src/librustc_lint/unused.rs
let mut err = cx.struct_span_lint(UNUSED_PARENS,
value.span,
&span_msg);
// Remove exactly one pair of parentheses (rather than naïvely

This comment has been minimized.

Sign in to view
Copy link
@estebank

estebank Sep 30, 2017

Contributor

@nikomatsakis do we care about unicode in rustc comments?

@zackmdavis zackmdavis force-pushed the zackmdavis:lint_suggestions branch from 15f4aa1 to 289faf5 Sep 30, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Sep 30, 2017 •
edited

(force-pushed to address formatting review comments)

@zackmdavis zackmdavis force-pushed the zackmdavis:lint_suggestions branch from 289faf5 to a399757 Sep 30, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Sep 30, 2017

(updated again to fix compile-fail failures reported by Travis)
@estebank

This comment has been minimized.

Sign in to view
Copy link
Contributor

estebank commented Oct 1, 2017

@bors r+

Thanks @zachreizner, great work!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 1, 2017

📌 Commit a399757 has been approved by estebank
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 1, 2017

⌛️ Testing commit a399757 with merge 7a52131...

bors added a commit that referenced this pull request Oct 1, 2017

@bors
Auto merge of #44942 - zackmdavis:lint_suggestions, r=estebank 
code suggestions for unused-mut, while-true, deprecated-attribute, and unused-parens lints

![lint_suggestions](https://user-images.githubusercontent.com/1076988/31044068-b2074de8-a57c-11e7-9319-6668508b6d1f.png)

r? @estebank
Loading status checks…
7a52131
@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Oct 1, 2017

Thanks @zachreizner, great work!

I agree; @zachreizner is the best! I especially like the one where he fixed the shift-right docs; it must have taken a very careful eye to catch that!

😉
@zachreizner

This comment has been minimized.

Sign in to view
Copy link
Contributor

zachreizner commented Oct 1, 2017

Not the first time I've been thanked in place of a a fellow Zach :-). I think my handle just comes up before the Zack Zach-s in the autocomplete.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 1, 2017

💔 Test failed - status-appveyor
@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Oct 1, 2017

Appveyor/Windows apparently doesn't like the --error-format json UI test. I think this is plausibly a bug in the test runner (rather than the JSON order being unstable or anything like that), since the only difference seems to be in the "file_name" value, which is supposed to be normalized (with "$DIR") and worked fine in the Travis/Unix-like tests, but for now I'll just rewrite it as a run-make test.

@zackmdavis zackmdavis force-pushed the zackmdavis:lint_suggestions branch from a399757 to fa8802a Oct 1, 2017

@estebank

This comment has been minimized.

Sign in to view
Copy link
Contributor

estebank commented Oct 1, 2017 •
edited

I believe we're really trying to move away from make tests. @zackmdavis, could you

  1. create a ticket to deal with the test output normalization failure on json output and
  2. revert your last change and add // ignore-windows to the test (this will make the test to be skipped under windows) with a comment linking to the previous ticket?

@zackmdavis zackmdavis referenced this pull request Oct 2, 2017

Closed

UI test expectation path normalization not working on Windows in all cases? #44968

@zackmdavis zackmdavis force-pushed the zackmdavis:lint_suggestions branch from fa8802a to 979671e Oct 2, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Oct 2, 2017

output normalization issue is #44968; updated tip of PR with ignore-windows is 979671e

@zackmdavis zackmdavis force-pushed the zackmdavis:lint_suggestions branch from 979671e to 8a14022 Oct 2, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Oct 2, 2017

correction, PR is 8a14022
@estebank

This comment has been minimized.

Sign in to view
Copy link
Contributor

estebank commented Oct 2, 2017

@bors r+ rollup
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 2, 2017

📌 Commit 8a14022 has been approved by estebank
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 2, 2017

⌛️ Testing commit 8a14022 with merge 9ae6ed7...

bors added a commit that referenced this pull request Oct 2, 2017

@bors
Auto merge of #44942 - zackmdavis:lint_suggestions, r=estebank 
code suggestions for unused-mut, while-true, deprecated-attribute, and unused-parens lints

![lint_suggestions](https://user-images.githubusercontent.com/1076988/31044068-b2074de8-a57c-11e7-9319-6668508b6d1f.png)

r? @estebank
Loading status checks…
9ae6ed7
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 9ae6ed7 to master...

@bors bors merged commit 8a14022 into rust-lang:master Oct 2, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

This was referenced Oct 11, 2017

Open
Merged

bors added a commit that referenced this pull request Oct 17, 2017

@bors
Auto merge of #45232 - zackmdavis:moar_lint_suggestions, r=estebank 
code suggestions for non-shorthand field pattern, no-mangle lints

continuing in the spirit of #44942

![moar_lint_suggestions](https://user-images.githubusercontent.com/1076988/31485011-3b20cc80-aee7-11e7-993d-81267ab77732.png)

r? @estebank
Loading status checks…
a2a91d7

bors added a commit that referenced this pull request Oct 18, 2017

@bors
Auto merge of #45232 - zackmdavis:moar_lint_suggestions, r=estebank 
code suggestions for non-shorthand field pattern, no-mangle lints

continuing in the spirit of #44942

![moar_lint_suggestions](https://user-images.githubusercontent.com/1076988/31485011-3b20cc80-aee7-11e7-993d-81267ab77732.png)

r? @estebank
Loading status checks…
ff33ecb

bors added a commit that referenced this pull request Oct 18, 2017

@bors
Auto merge of #45232 - zackmdavis:moar_lint_suggestions, r=estebank 
code suggestions for non-shorthand field pattern, no-mangle lints

continuing in the spirit of #44942

![moar_lint_suggestions](https://user-images.githubusercontent.com/1076988/31485011-3b20cc80-aee7-11e7-993d-81267ab77732.png)

r? @estebank
Loading status checks…
395388b

bors added a commit that referenced this pull request Oct 19, 2017

@bors
Auto merge of #45232 - zackmdavis:moar_lint_suggestions, r=estebank 
code suggestions for non-shorthand field pattern, no-mangle lints

continuing in the spirit of #44942

![moar_lint_suggestions](https://user-images.githubusercontent.com/1076988/31485011-3b20cc80-aee7-11e7-993d-81267ab77732.png)

r? @estebank
Loading status checks…
e3fb84e

@zackmdavis zackmdavis deleted the zackmdavis:lint_suggestions branch Jan 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.