fix a bunch of clippy warnings #5876

matthiaskrgr · 2018-08-08T23:01:16Z

(invocation: cargo clippy --all-targets --all-features -- --cap-lints warn )

rust-highfive · 2018-08-08T23:01:20Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

dwijnand

Nice! A few (simple) comments and questions from me.

dwijnand · 2018-08-09T06:09:00Z

tests/testsuite/profile_overrides.rs

@@ -202,7 +202,7 @@ fn profile_override_bad_settings() {
        ),
        ("overrides = {}", "Profile overrides cannot be nested."),
    ];
-    for &(ref snippet, ref expected) in bad_values.iter() {
+    for &(ref snippet, ref expected) in &bad_values {


There's a preference in Cargo to use the former over the latter.

Ok, I added a allow(explicit_iter_loop) to testsuites main.rs.

dwijnand · 2018-08-09T06:15:21Z

tests/testsuite/support/mod.rs

@@ -1255,7 +1255,7 @@ pub fn basic_lib_manifest(name: &str) -> String {
    )
 }

-pub fn path2url(p: PathBuf) -> Url {
+pub fn path2url(p: &PathBuf) -> Url {


Would the use-cases stay simple if this took a p: P where P: AsRef<Path>?

dwijnand · 2018-08-09T06:17:20Z

tests/testsuite/config.rs

@@ -292,7 +292,7 @@ opt-level = 'foo'

    let config = new_config(&[]);

-    assert_error(
+    assert_error(&
        config.get::<toml::TomlProfile>("profile.dev").unwrap_err(),


Is this formatting common? Looks strange to me.

Ah oops, fallout of lazy search and replace :/

dwijnand · 2018-08-09T06:31:03Z

tests/testsuite/config.rs

@@ -68,7 +68,7 @@ fn new_config(env: &[(&str, &str)]) -> Config {
    config
 }

-fn assert_error(error: CargoError, msgs: &str) {
+fn assert_error(error: &CargoError, msgs: &str) {


Similarly, would using AsRef<CargoError> avoid changing all the existing use sites?

Hmm I could not get these and the path2url to work with AsRef, but I can revert the change altogether if you want.

I'll take a swing at it.

Here's the change that worked for me on path2url:

-pub fn path2url(p: PathBuf) -> Url { - Url::from_file_path(&*p).ok().unwrap() +pub fn path2url<P: AsRef<Path>>(p: P) -> Url { + Url::from_file_path(p).ok().unwrap() }

For assert_error:

use cargo::CargoError; use support::{execs, lines_match, paths, project}; use support::hamcrest::assert_that; +use std::borrow::Borrow; use std::collections; use std::fs;

-fn assert_error(error: CargoError, msgs: &str) { +fn assert_error<E: Borrow<CargoError>>(error: E, msgs: &str) { let causes = error + .borrow() .iter_chain() .map(|e| e.to_string()) .collect::<Vec<_>>()

Thanks a lot!

dwijnand · 2018-08-09T10:37:46Z

tests/testsuite/build.rs

            "\
 [COMPILING] present_dep v1.2.3
 [COMPILING] bar v0.1.0 ([..])
-[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]"
-        )),
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]".to_string()


these to_string()'s aren't necessary as with_stderr takes S: ToString, and thus can take a string slice.

Thanks, updated.

dwijnand

LGTM. Thanks @matthiaskrgr!

matthiaskrgr · 2018-08-10T21:58:26Z

Thanks for the reviews!

alexcrichton · 2018-08-12T04:34:40Z

src/cargo/ops/fix.rs

-                    "2018" => { cmd.arg("-Wrust-2018-idioms"); }
-                    _ => {}
-                }
+                if let "2018" = &edition[..] { cmd.arg("-Wrust-2018-idioms"); }


Could this just be if edition == "2018" { ... }?

Indeed, updated accordingly.

…s --all-features -- --cap-lints warn ) Special thanks to dwijnand for helping me with this! :)

matthiaskrgr · 2018-08-12T08:41:07Z

CI is flakey again....

info: downloading component 'rust-std' for 'i686-unknown-linux-gnu'
error: component download failed for rust-std-i686-unknown-linux-gnu
info: caused by: could not download file from 'https://static.rust-lang.org/dist/2018-08-10/rust-std-beta-i686-unknown-linux-gnu.tar.xz' to '/home/travis/.rustup/downloads/e37304d2a8920fcefa8f7f9c8af01c7ba4fe7dddb0cdf0876554b61a1702ff80.partial'
info: caused by: error during download
info: caused by: [28] Timeout was reached (Connection timed out after 30001 milliseconds)

alexcrichton · 2018-08-12T22:42:50Z

@bors: r+

bors · 2018-08-12T22:42:51Z

📌 Commit 8798bf0 has been approved by alexcrichton

bors · 2018-08-12T22:42:58Z

⌛ Testing commit 8798bf0 with merge ae97799...

fix a bunch of clippy warnings (invocation: cargo clippy --all-targets --all-features -- --cap-lints warn )

bors · 2018-08-12T23:22:14Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ae97799 to master...

Update cargo - Update transitioning url (rust-lang/cargo#5889) - Resolve some clippy lint warnings (rust-lang/cargo#5884) - Don't kill child processes on normal exit on Windows (rust-lang/cargo#5887) - fix a bunch of clippy warnings (rust-lang/cargo#5876) - Add support for rustc's --error-format short (rust-lang/cargo#5879) - Support JSON with rustdoc. (rust-lang/cargo#5878) - Fix rustfmt instructions in CONTRIBUTING.md (rust-lang/cargo#5880) - Allow `cargo run` in workspaces. (rust-lang/cargo#5877) - Change target filters in workspaces. (rust-lang/cargo#5873) - Improve verbose console and log for finding git repo in package check (rust-lang/cargo#5858) - Meta rename (rust-lang/cargo#5871) - fetch: skip target tests when cross_compile is disabled (rust-lang/cargo#5870) - Fully capture rustc and rustdoc output when -Zcompile-progress is passed (rust-lang/cargo#5862) - Fix test --example docs. (rust-lang/cargo#5867) - Add a feature to build a vendored OpenSSL (rust-lang/cargo#5865)

rust-highfive assigned matklad Aug 8, 2018

dwijnand reviewed Aug 9, 2018

View reviewed changes

matthiaskrgr force-pushed the clippy_2 branch from 46f208e to 5fc5748 Compare August 9, 2018 07:46

dwijnand reviewed Aug 9, 2018

View reviewed changes

matthiaskrgr force-pushed the clippy_2 branch from 5fc5748 to 008ea20 Compare August 10, 2018 20:08

dwijnand approved these changes Aug 10, 2018

View reviewed changes

alexcrichton reviewed Aug 12, 2018

View reviewed changes

matthiaskrgr force-pushed the clippy_2 branch from 008ea20 to c206657 Compare August 12, 2018 05:04

fix a bunch of clippy warnings (invocation: cargo clippy --all-target…

8798bf0

…s --all-features -- --cap-lints warn ) Special thanks to dwijnand for helping me with this! :)

matthiaskrgr force-pushed the clippy_2 branch from c206657 to 8798bf0 Compare August 12, 2018 08:00

dwijnand mentioned this pull request Aug 12, 2018

https://static.rust-lang.org/dist/ is flakey rust-lang/infra-team#16

Closed

bors added a commit that referenced this pull request Aug 12, 2018

Auto merge of #5876 - matthiaskrgr:clippy_2, r=alexcrichton

ae97799

fix a bunch of clippy warnings (invocation: cargo clippy --all-targets --all-features -- --cap-lints warn )

bors merged commit 8798bf0 into rust-lang:master Aug 12, 2018

ehuss mentioned this pull request Aug 14, 2018

Update cargo rust-lang/rust#53367

Merged

matthiaskrgr deleted the clippy_2 branch September 3, 2018 08:06

ehuss added this to the 1.30.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bunch of clippy warnings #5876

fix a bunch of clippy warnings #5876

matthiaskrgr commented Aug 8, 2018

rust-highfive commented Aug 8, 2018

dwijnand left a comment

dwijnand Aug 9, 2018

matthiaskrgr Aug 9, 2018

dwijnand Aug 9, 2018

dwijnand Aug 9, 2018

matthiaskrgr Aug 9, 2018

dwijnand Aug 9, 2018

matthiaskrgr Aug 9, 2018

dwijnand Aug 9, 2018

dwijnand Aug 9, 2018

dwijnand Aug 9, 2018

matthiaskrgr Aug 10, 2018

dwijnand Aug 9, 2018

matthiaskrgr Aug 10, 2018

dwijnand left a comment

matthiaskrgr commented Aug 10, 2018

alexcrichton Aug 12, 2018

matthiaskrgr Aug 12, 2018

matthiaskrgr commented Aug 12, 2018

alexcrichton commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 12, 2018

fix a bunch of clippy warnings #5876

fix a bunch of clippy warnings #5876

Conversation

matthiaskrgr commented Aug 8, 2018

rust-highfive commented Aug 8, 2018

dwijnand left a 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

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

Choose a reason for hiding this comment

dwijnand left a comment

Choose a reason for hiding this comment

matthiaskrgr commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiaskrgr commented Aug 12, 2018

alexcrichton commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 12, 2018