Skip to content

Fix up some issues flagged by clippy #294

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

Merged
merged 7 commits into from
May 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions fuzz/fuzzers/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ extern crate url;
use std::str;

fuzz_target!(|data: &[u8]| {
if let Ok(utf8) = str::from_utf8(data) {
let url = url::Url::parse(utf8);
}
if let Ok(utf8) = str::from_utf8(data) {
let _ = url::Url::parse(utf8);
}
});
2 changes: 1 addition & 1 deletion src/form_urlencoded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ fn decode(input: &[u8], encoding: EncodingOverride) -> Cow<str> {
}

/// Replace b'+' with b' '
fn replace_plus<'a>(input: &'a [u8]) -> Cow<'a, [u8]> {
fn replace_plus(input: &[u8]) -> Cow<[u8]> {
match input.iter().position(|&b| b == b'+') {
None => Cow::Borrowed(input),
Some(first_position) => {
Expand Down
14 changes: 6 additions & 8 deletions src/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ impl Host<String> {
///
/// https://url.spec.whatwg.org/#host-parsing
pub fn parse(input: &str) -> Result<Self, ParseError> {
if input.starts_with("[") {
if !input.ends_with("]") {
if input.starts_with('[') {
if !input.ends_with(']') {
return Err(ParseError::InvalidIpv6Address)
}
return parse_ipv6addr(&input[1..input.len() - 1]).map(Host::Ipv6)
Expand Down Expand Up @@ -304,17 +304,17 @@ fn parse_ipv4number(mut input: &str) -> Result<u32, ()> {
if input.starts_with("0x") || input.starts_with("0X") {
input = &input[2..];
r = 16;
} else if input.len() >= 2 && input.starts_with("0") {
} else if input.len() >= 2 && input.starts_with('0') {
input = &input[1..];
r = 8;
}
if input.is_empty() {
return Ok(0);
}
if input.starts_with("+") {
if input.starts_with('+') {
return Err(())
}
match u32::from_str_radix(&input, r) {
match u32::from_str_radix(input, r) {
Ok(number) => Ok(number),
Err(_) => Err(()),
}
Expand Down Expand Up @@ -477,9 +477,7 @@ fn parse_ipv6addr(input: &str) -> ParseResult<Ipv6Addr> {
let mut swaps = piece_pointer - compress_pointer;
piece_pointer = 7;
while swaps > 0 {
let tmp = pieces[piece_pointer];
pieces[piece_pointer] = pieces[compress_pointer + swaps - 1];
pieces[compress_pointer + swaps - 1] = tmp;
pieces.swap(piece_pointer, compress_pointer + swaps - 1);
Copy link

@lucab lucab May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly out of scope of this PR, but I think both parameters would benefit from some bound checking to ensure no index underflow/overflow will induce a panic here at runtime.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make a separate issue for this? Thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #334.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this still appears as part of the clippy related changes.

Can you move this into a new branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clippy related change. Bounds checking is a separate issue being tracked in #334.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Then this PR looks OK to merge to me 👍

swaps -= 1;
piece_pointer -= 1;
}
Expand Down
18 changes: 9 additions & 9 deletions src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ impl<F: FnMut(char) -> bool> Pattern for F {
impl<'i> Iterator for Input<'i> {
type Item = char;
fn next(&mut self) -> Option<char> {
self.chars.by_ref().filter(|&c| !matches!(c, '\t' | '\n' | '\r')).next()
self.chars.by_ref().find(|&c| !matches!(c, '\t' | '\n' | '\r'))
}
}

Expand Down Expand Up @@ -736,8 +736,8 @@ impl<'a> Parser<'a> {
Ok((host_end, host.into(), port, remaining))
}

pub fn parse_host<'i>(mut input: Input<'i>, scheme_type: SchemeType)
-> ParseResult<(Host<String>, Input<'i>)> {
pub fn parse_host(mut input: Input, scheme_type: SchemeType)
-> ParseResult<(Host<String>, Input)> {
// Undo the Input abstraction here to avoid allocating in the common case
// where the host part of the input does not contain any tab or newline
let input_str = input.chars.as_str();
Expand Down Expand Up @@ -830,9 +830,9 @@ impl<'a> Parser<'a> {
Ok((true, host, remaining))
}

pub fn parse_port<'i, P>(mut input: Input<'i>, default_port: P,
pub fn parse_port<P>(mut input: Input, default_port: P,
context: Context)
-> ParseResult<(Option<u16>, Input<'i>)>
-> ParseResult<(Option<u16>, Input)>
where P: Fn() -> Option<u16> {
let mut port: u32 = 0;
let mut has_any_digit = false;
Expand All @@ -854,7 +854,7 @@ impl<'a> Parser<'a> {
if !has_any_digit || opt_port == default_port() {
opt_port = None;
}
return Ok((opt_port, input))
Ok((opt_port, input))
}

pub fn parse_path_start<'i>(&mut self, scheme_type: SchemeType, has_host: &mut bool,
Expand All @@ -878,7 +878,7 @@ impl<'a> Parser<'a> {
path_start: usize, mut input: Input<'i>)
-> Input<'i> {
// Relative path state
debug_assert!(self.serialization.ends_with("/"));
debug_assert!(self.serialization.ends_with('/'));
loop {
let segment_start = self.serialization.len();
let mut ends_with_slash = false;
Expand Down Expand Up @@ -926,7 +926,7 @@ impl<'a> Parser<'a> {
debug_assert!(self.serialization.as_bytes()[segment_start - 1] == b'/');
self.serialization.truncate(segment_start - 1); // Truncate "/.."
self.pop_path(scheme_type, path_start);
if !self.serialization[path_start..].ends_with("/") {
if !self.serialization[path_start..].ends_with('/') {
self.serialization.push('/')
}
},
Expand Down Expand Up @@ -1030,7 +1030,7 @@ impl<'a> Parser<'a> {
}
}
None => return Ok((None, None)),
_ => panic!("Programming error. parse_query_and_fragment() called without ? or # {:?}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strange. Why was the {:?} there?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this would give a compiler error. Weird.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Me too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compiles because the panic! macro has a special case for use with a single "argument" that by-passes the string formatting system entirely:

https://github.com/rust-lang/rust/blob/a0da1e0653/src/libstd/macros.rs#L43-L49

_ => panic!("Programming error. parse_query_and_fragment() called without ? or #")
}

let fragment_start = to_u32(self.serialization.len())?;
Expand Down
4 changes: 2 additions & 2 deletions src/percent_encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl<'a, E: EncodeSet> From<PercentEncode<'a, E>> for Cow<'a, str> {
/// (which returns `Cow::Borrowed` when `input` contains no percent-encoded sequence)
/// and has `decode_utf8()` and `decode_utf8_lossy()` methods.
#[inline]
pub fn percent_decode<'a>(input: &'a [u8]) -> PercentDecode<'a> {
pub fn percent_decode(input: &[u8]) -> PercentDecode {
PercentDecode {
bytes: input.iter()
}
Expand Down Expand Up @@ -298,7 +298,7 @@ impl<'a> PercentDecode<'a> {
/// If the percent-decoding is different from the input, return it as a new bytes vector.
pub fn if_any(&self) -> Option<Vec<u8>> {
let mut bytes_iter = self.bytes.clone();
while bytes_iter.find(|&&b| b == b'%').is_some() {
while bytes_iter.any(|&b| b == b'%') {
if let Some(decoded_byte) = after_percent_sign(&mut bytes_iter) {
let initial_bytes = self.bytes.as_slice();
let unchanged_bytes_len = initial_bytes.len() - bytes_iter.len() - 3;
Expand Down
4 changes: 2 additions & 2 deletions tests/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn check_invariants(url: &Url) {
}


fn run_parsing(input: String, base: String, expected: Result<ExpectedAttributes, ()>) {
fn run_parsing(input: &str, base: &str, expected: Result<ExpectedAttributes, ()>) {
let base = match Url::parse(&base) {
Ok(base) => base,
Err(message) => panic!("Error parsing base {:?}: {}", base, message)
Expand Down Expand Up @@ -135,7 +135,7 @@ fn collect_parsing<F: FnMut(String, test::TestFn)>(add_test: &mut F) {
})
};
add_test(format!("{:?} @ base {:?}", input, base),
test::TestFn::dyn_test_fn(move || run_parsing(input, base, expected)));
test::TestFn::dyn_test_fn(move || run_parsing(&input, &base, expected)));
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,12 @@ fn test_equality() {
// Different scheme
let a: Url = url("http://example.com/");
let b: Url = url("https://example.com/");
assert!(a != b);
assert_ne!(a, b);

// Different host
let a: Url = url("http://foo.com/");
let b: Url = url("http://bar.com/");
assert!(a != b);
assert_ne!(a, b);

// Missing path, automatically substituted. Semantically the same.
let a: Url = url("http://foo.com");
Expand Down