Apply clippy suggestions for rustc and core #89709

clemenswasser · 2021-10-09T18:46:56Z

No description provided.

rust-highfive · 2021-10-09T18:46:58Z

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

rust-highfive · 2021-10-09T18:46:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

compiler/rustc_index/src/bit_set.rs

compiler/rustc_apfloat/src/ieee.rs

eddyb · 2021-10-10T08:48:38Z

compiler/rustc_data_structures/src/base_n.rs

@@ -14,7 +14,7 @@ const BASE_64: &[u8; MAX_BASE as usize] =

 #[inline]
 pub fn push_str(mut n: u128, base: usize, output: &mut String) {
-    debug_assert!(base >= 2 && base <= MAX_BASE);
+    debug_assert!((2..=MAX_BASE).contains(&base));


It would be nice if we could use debug_assert_matches! (assuming it exists) for range checks.

debug_assert_matches! is currently unstable

Compiler code can use unstable library features.

@clemenswasser If this PR is re-landed (after the #89905 revert), it would be good to fix this.

eddyb · 2021-10-10T08:50:39Z

compiler/rustc_macros/src/session_diagnostic.rs

@@ -388,7 +388,7 @@ impl<'a> SessionDiagnosticDeriveBuilder<'a> {
                let formatted_str = self.build_format(&s.value(), attr.span());
                match name {
                    "message" => {
-                        if type_matches_path(&info.ty, &["rustc_span", "Span"]) {
+                        if type_matches_path(info.ty, &["rustc_span", "Span"]) {


Nice, removing redundant & should maybe be more forcefully handled (i.e. denyed in rustc crates) - and if you open a PR with just that lint being accounted for, I will r+ it immediately.

Is there a simple way I can deny the lints for all rustc crates, without copy pasting in all lib.rs?

In theory we could simply deny the lint via x.py clippy, but in reality it seems that --cap-lints warn prevents this from working 😆

diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 28e7f1fdca7..cf21a4dc857 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -44,6 +44,7 @@ fn strings<'a>(arr: &'a [&str]) -> impl Iterator<Item = String> + 'a { } args.extend(strings(&["--", "--cap-lints", "warn"])); args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint))); + args.push("-Dclippy::needless_borrow".into()); args } else { vec![]

library/core/src/num/flt2dec/strategy/grisu.rs

compiler/rustc_serialize/src/serialize.rs

compiler/rustc_data_structures/src/stack.rs

compiler/rustc_data_structures/src/obligation_forest/mod.rs

src/bootstrap/builder.rs

compiler/rustc_serialize/src/serialize.rs

compiler/rustc_data_structures/src/obligation_forest/mod.rs

compiler/rustc_index/src/bit_set.rs

petrochenkov · 2021-10-10T20:05:10Z

r=me with #89709 (comment) addressed.

petrochenkov · 2021-10-11T08:21:20Z

@bors r+

bors · 2021-10-11T08:21:21Z

📌 Commit 14b6cf6 has been approved by petrochenkov

matthiaskrgr · 2021-10-11T10:00:05Z

@bors rollup=iffy just in case this has some unforeseen perf implications

bors · 2021-10-11T11:14:50Z

⌛ Testing commit 14b6cf6 with merge 6ae8912...

bors · 2021-10-11T14:16:08Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 6ae8912 to master...

rust-timer · 2021-10-11T15:49:47Z

Finished benchmarking commit (6ae8912): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.8% on full builds of keccak)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

pnkfelix · 2021-10-12T21:09:06Z

@bors rollup=iffy just in case this has some unforeseen perf implications

I think its fair to say this had some unforeseen perf implications

We see instruction-count regressions in the 1.1% to 2.7% range in keccak and inflate

petrochenkov · 2021-10-13T15:50:30Z

I suggest reverting.
The changes are non-critical, and I'm not sure whether @clemenswasser is going to investigate the regressions (I'm certainly not going to do that myself).

matthiaskrgr · 2021-10-13T20:05:47Z

I reverted parts of the merge in #89855 can we get a perf run there to see if that fixes the regression?

CAD97 · 2021-10-14T05:58:03Z

compiler/rustc_data_structures/src/sso/map.rs

-                } else {
-                    None
-                }
+                array.iter().position(|(k, _v)| k == key).map(|index| array.swap_remove(index).1)


I know other data structures have some instances of open coding map to reduce the amount of monomorphization bloat that needs to be compiled, so this is a potential perf culprit

I would worry less about bloat and more about that instance of Option::map having the closure inlined into it, but not being inlined itself (because of inlining swap_remove?).

…ggestions_2, r=petrochenkov" The PR had some unforseen perf regressions that are not as easy to find. Revert the PR for now. This reverts commit 6ae8912, reversing changes made to 86d6d2b.

eddyb · 2021-10-15T12:06:56Z

compiler/rustc_data_structures/src/obligation_forest/mod.rs

-        for index in 0..orig_nodes_len {
+        for (index, node_rewrite) in node_rewrites.iter_mut().enumerate() {


Is it possible enumerate is less efficient than range iteration?

eddyb · 2021-10-15T12:09:16Z

compiler/rustc_index/src/bit_set.rs

-        let words = &mut self.words[..];
-        for index in start..end {
-            words[index] = !0;
+        for word in self.words[start..end].iter_mut() {


I didn't get to this in time, but xs[range].iter_mut() should just be writen &mut xs[range] - surprised clippy doesn't have a lint for this.

eddyb · 2021-10-15T12:10:09Z

compiler/rustc_index/src/bit_set.rs

-        let words = &mut self.words[..];
-        for index in start..end {
-            words[index] = !0;
+        for word in self.words[start..end].iter_mut() {
+            *word = !0;
        }


This entire thing can just be self.words[start..end].fill(!0), just realized.

…chaelwoerister Revert "Auto merge of rust-lang#89709 - clemenswasser:apply_clippy_suggestions… …_2, r=petrochenkov" The PR had some unforseen perf regressions that are not as easy to find. Revert the PR for now. This reverts commit 6ae8912, reversing changes made to 86d6d2b.

rylev · 2021-10-20T08:37:58Z

As noted many of these changes have been reverted, and the performance was regained. As such I'm going to mark this as triaged.

@rustbot label +perf-regression-triaged

rust-highfive assigned petrochenkov Oct 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2021

This comment has been minimized.

Sign in to view

clemenswasser force-pushed the apply_clippy_suggestions_2 branch from a08bdbb to 1141998 Compare October 9, 2021 19:00

fbstj reviewed Oct 9, 2021

View reviewed changes

compiler/rustc_index/src/bit_set.rs Outdated Show resolved Hide resolved

clemenswasser force-pushed the apply_clippy_suggestions_2 branch from 1141998 to 0e8704d Compare October 9, 2021 19:28

eddyb suggested changes Oct 10, 2021

View reviewed changes

clemenswasser force-pushed the apply_clippy_suggestions_2 branch from 0e8704d to 3111c12 Compare October 10, 2021 10:13

fbstj reviewed Oct 10, 2021

View reviewed changes

src/bootstrap/builder.rs Outdated Show resolved Hide resolved

clemenswasser force-pushed the apply_clippy_suggestions_2 branch from 3111c12 to a669733 Compare October 10, 2021 13:04

klensy reviewed Oct 10, 2021

View reviewed changes

compiler/rustc_serialize/src/serialize.rs Outdated Show resolved Hide resolved

petrochenkov 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 Oct 10, 2021

Apply clippy suggestions

71dd0b9

clemenswasser force-pushed the apply_clippy_suggestions_2 branch from a669733 to 71dd0b9 Compare October 10, 2021 13:38

klensy reviewed Oct 10, 2021

View reviewed changes

compiler/rustc_data_structures/src/obligation_forest/mod.rs Outdated Show resolved Hide resolved

Remove for loop range

b8c151c

LingMan reviewed Oct 10, 2021

View reviewed changes

compiler/rustc_index/src/bit_set.rs Outdated Show resolved Hide resolved

Remove unnecessary variable

14b6cf6

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 Oct 11, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 11, 2021

bors merged commit 6ae8912 into rust-lang:master Oct 11, 2021

rustbot added this to the 1.57.0 milestone Oct 11, 2021

rustbot added the perf-regression Performance regression. label Oct 11, 2021

matthiaskrgr mentioned this pull request Oct 13, 2021

revert parts of #89709 (Apply clippy suggestions for rustc and core) to see if it mitigates the perf hit #89855

Closed

CAD97 reviewed Oct 14, 2021

View reviewed changes

eddyb reviewed Oct 15, 2021

View reviewed changes

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply clippy suggestions for rustc and core #89709

Apply clippy suggestions for rustc and core #89709

clemenswasser commented Oct 9, 2021

rust-highfive commented Oct 9, 2021

rust-highfive commented Oct 9, 2021

This comment has been minimized.

eddyb Oct 10, 2021

clemenswasser Oct 10, 2021

petrochenkov Oct 10, 2021

eddyb Oct 15, 2021

eddyb Oct 10, 2021

clemenswasser Oct 10, 2021

matthiaskrgr Oct 10, 2021

petrochenkov commented Oct 10, 2021

petrochenkov commented Oct 11, 2021

bors commented Oct 11, 2021

matthiaskrgr commented Oct 11, 2021

bors commented Oct 11, 2021

bors commented Oct 11, 2021

rust-timer commented Oct 11, 2021

pnkfelix commented Oct 12, 2021

petrochenkov commented Oct 13, 2021

matthiaskrgr commented Oct 13, 2021

CAD97 Oct 14, 2021

eddyb Oct 15, 2021

eddyb Oct 15, 2021

eddyb Oct 15, 2021

eddyb Oct 15, 2021

rylev commented Oct 20, 2021

		for index in 0..orig_nodes_len {
		for (index, node_rewrite) in node_rewrites.iter_mut().enumerate() {

Apply clippy suggestions for rustc and core #89709

Apply clippy suggestions for rustc and core #89709

Conversation

clemenswasser commented Oct 9, 2021

rust-highfive commented Oct 9, 2021

rust-highfive commented Oct 9, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 10, 2021

petrochenkov commented Oct 11, 2021

bors commented Oct 11, 2021

matthiaskrgr commented Oct 11, 2021

bors commented Oct 11, 2021

bors commented Oct 11, 2021

rust-timer commented Oct 11, 2021

pnkfelix commented Oct 12, 2021

petrochenkov commented Oct 13, 2021

matthiaskrgr commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Oct 20, 2021