Repository: algesten/ureq
Pull request #405 (2021-08-11 07:09:44)

Fix clippy warnings
1. Fix linter warning from clippy about unnecessary borrows - "This expression borrows a reference ... that is immediately dereferenced by the compiler"
2. Replace explicit loop with iterator operations in Header to avoid unnecessary mutation

algesten: Hi! Welcome to ureq!

Thank you for the PR, but I think we need to split the work up. Fixing the clippy lint warnings is good, maybe we can reshape this PR to do only that?

Replacing the for-loops with iterators, I'm not so sure about. Typically I find for loops more readable than long iterator chains, and in this specific instance, I think you will end up with worse performance.

The for loop both checks for `:` and illegal chars at the same time.

```rust
        let bytes = self.as_bytes();
        let index = bytes.iter().position(|x| *x == b':').unwrap_or(bytes.len());

        let name_bytes = &bytes[..index];
        let bad = name_bytes.partition_point(is_tchar);
```

This however uses two iterators (`position()` and `partition_point()`) to do the same thing. Reading the docs, I also don't think the use of `partition_point` is correct here.

So to sum it up:

* If you change this PR to addressing the clippy lints only, I'm happy to land that.
* If you want to explore iterators further, please open a separate PR, but beware that I have a mild preference against them, and that I don't want to accept anything that makes performance worse.
nmurali94: > Fixing the clippy lint warnings is good, maybe we can reshape this PR to do only that?

Will do!

> This however uses two iterators (position() and partition_point()) to do the same thing

My view here was that while we have two iterations here, we're doing half the work in each step so we shouldn't be doing much more work. Plus without a branch check in each step, my intuition says the two iterators can be better optimized by the compiler. But since I don't have anything concrete to show here, I'll defer to your judgement
algesten: > My view here was that while we have two iterations here, we're doing half the work in each step so we shouldn't be doing much more work. Plus without a branch check in each step, my intuition says the two iterators can be better optimized by the compiler. But since I don't have anything concrete to show here, I'll defer to your judgement

You could be right. I can be convinced by science here ;) 

Patch:
diff --git a/src/body.rs b/src/body.rs
index 6420bef..1711e5c 100644
--- a/src/body.rs
+++ b/src/body.rs
@@ -134,7 +134,7 @@ fn copy_chunked<R: Read, W: Write>(reader: &mut R, writer: &mut W) -> io::Result
         let header = header_str.as_bytes();
         assert!(header.len() <= CHUNK_HEADER_MAX_SIZE);
         let start_index = CHUNK_HEADER_MAX_SIZE - header.len();
-        (&mut chunk[start_index..]).write_all(&header).unwrap();
+        (&mut chunk[start_index..]).write_all(header).unwrap();
 
         // And add the footer
         chunk.extend_from_slice(b"\r\n");
diff --git a/src/pool.rs b/src/pool.rs
index dcb73ec..019692f 100644
--- a/src/pool.rs
+++ b/src/pool.rs
@@ -113,7 +113,7 @@ impl ConnectionPool {
 
                 // Remove the newest matching PoolKey from self.lru. That
                 // corresponds to the stream we just removed from `recycle`.
-                remove_last_match(&mut inner.lru, &key)
+                remove_last_match(&mut inner.lru, key)
                     .expect("invariant failed: key in recycle but not in lru");
 
                 debug!("pulling stream from pool: {:?} -> {:?}", key, stream);
diff --git a/src/response.rs b/src/response.rs
index c7b3a8a..1f7fc15 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -134,7 +134,7 @@ impl Response {
     /// The HTTP spec allows for non-utf8 status texts. This uses from_utf8_lossy to
     /// convert such lines to &str.
     pub fn status_text(&self) -> &str {
-        &self.status_line.as_str()[self.index.response_code + 1..].trim()
+        self.status_line.as_str()[self.index.response_code + 1..].trim()
     }
 
     /// The header value for the given name, or None if not found.
diff --git a/src/stream.rs b/src/stream.rs
index fd4b5bd..3563806 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -209,7 +209,7 @@ impl Stream {
         match self.inner.get_ref() {
             Inner::Http(b) => Some(b),
             #[cfg(feature = "tls")]
-            Inner::Https(b) => Some(&b.get_ref()),
+            Inner::Https(b) => Some(b.get_ref()),
             _ => None,
         }
     }
@@ -365,7 +365,7 @@ pub(crate) fn connect_https(unit: &Unit, hostname: &str) -> Result<Stream, Error
         .map(|c| &c.0)
         .unwrap_or(&*TLS_CONF);
     let mut sock = connect_host(unit, hostname, port)?;
-    let mut sess = rustls::ClientSession::new(&tls_conf, sni);
+    let mut sess = rustls::ClientSession::new(tls_conf, sni);
 
     sess.complete_io(&mut sock)
         .map_err(|err| ErrorKind::ConnectionFailed.new().src(err))?;
@@ -413,7 +413,7 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
         // connect with a configured timeout.
         let stream = if Some(Proto::SOCKS5) == proto {
             connect_socks5(
-                &unit,
+                unit,
                 proxy.clone().unwrap(),
                 connect_deadline,
                 sock_addr,
diff --git a/src/unit.rs b/src/unit.rs
index 063cca4..618d924 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -42,7 +42,7 @@ impl Unit {
     ) -> Self {
         //
 
-        let (is_transfer_encoding_set, mut is_chunked) = get_header(&headers, "transfer-encoding")
+        let (is_transfer_encoding_set, mut is_chunked) = get_header(headers, "transfer-encoding")
             // if the user has set an encoding header, obey that.
             .map(|enc| {
                 let is_transfer_encoding_set = !enc.is_empty();
@@ -60,7 +60,7 @@ impl Unit {
 
             // chunking and Content-Length headers are mutually exclusive
             // also don't write this if the user has set it themselves
-            if !is_chunked && get_header(&headers, "content-length").is_none() {
+            if !is_chunked && get_header(headers, "content-length").is_none() {
                 // if the payload is of known size (everything beside an unsized reader), set
                 // Content-Length,
                 // otherwise, use the chunked Transfer-Encoding (only if no other Transfer-Encoding
@@ -82,7 +82,7 @@ impl Unit {
             let username = url.username();
             let password = url.password().unwrap_or("");
             if (!username.is_empty() || !password.is_empty())
-                && get_header(&headers, "authorization").is_none()
+                && get_header(headers, "authorization").is_none()
             {
                 let encoded = base64::encode(&format!("{}:{}", username, password));
                 extra.push(Header::new("Authorization", &format!("Basic {}", encoded)));
@@ -236,7 +236,7 @@ fn connect_inner(
     let url = &unit.url;
     let method = &unit.method;
     // open socket
-    let (mut stream, is_recycled) = connect_socket(&unit, &host, use_pooled)?;
+    let (mut stream, is_recycled) = connect_socket(unit, host, use_pooled)?;
 
     if is_recycled {
         info!("sending request (reused connection) {} {}", method, url);
@@ -244,7 +244,7 @@ fn connect_inner(
         info!("sending request {} {}", method, url);
     }
 
-    let send_result = send_prelude(&unit, &mut stream, !previous.is_empty());
+    let send_result = send_prelude(unit, &mut stream, !previous.is_empty());
 
     if let Err(err) = send_result {
         if is_recycled {
@@ -343,9 +343,9 @@ fn connect_socket(unit: &Unit, hostname: &str, use_pooled: bool) -> Result<(Stre
         }
     }
     let stream = match unit.url.scheme() {
-        "http" => stream::connect_http(&unit, hostname),
-        "https" => stream::connect_https(&unit, hostname),
-        "test" => connect_test(&unit),
+        "http" => stream::connect_http(unit, hostname),
+        "https" => stream::connect_https(unit, hostname),
+        "test" => connect_test(unit),
         scheme => Err(ErrorKind::UnknownScheme.msg(&format!("unknown scheme {}", scheme))),
     };
     Ok((stream?, false))


Test patch:
diff --git a/src/testserver.rs b/src/testserver.rs
index 930a559..5df8f6b 100644
--- a/src/testserver.rs
+++ b/src/testserver.rs
@@ -85,7 +85,7 @@ impl TestHeaders {
         if self.0.is_empty() {
             ""
         } else {
-            &self.0[0].split(' ').nth(1).unwrap()
+            self.0[0].split(' ').nth(1).unwrap()
         }
     }