Skip to content

/ rust-url

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

Fix up some issues flagged by clippy #294

Merged
merged 7 commits into from May 20, 2017
Merged

Fix up some issues flagged by clippy #294

merged 7 commits into from May 20, 2017
+25 −27

Conversation

@daniellockyer
Copy link
Contributor

daniellockyer commented Apr 19, 2017 •
edited by larsbergstrom

This change is Reviewable
@lucab
lucab reviewed May 9, 2017
View changes
src/host.rs
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);

This comment has been minimized.

Sign in to view
@lucab

lucab May 9, 2017 •
edited

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.

This comment has been minimized.

Sign in to view
@Hoverbear

Hoverbear May 10, 2017

Member

Could you make a separate issue for this? Thanks!

This comment has been minimized.

Sign in to view
@lucab

lucab May 10, 2017

Moved to #334.

This comment has been minimized.

Sign in to view
@luisbg

luisbg May 16, 2017

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

Can you move this into a new branch?

This comment has been minimized.

Sign in to view
@dtolnay

dtolnay May 16, 2017

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

This comment has been minimized.

Sign in to view
@luisbg

luisbg May 16, 2017

Cool! Then this PR looks OK to merge to me 👍
@vks
Copy link

vks commented May 9, 2017

What is the minimum Rust version that should be supported by rust-url?
@vks
vks reviewed May 9, 2017
View changes
tests/data.rs Outdated
@@ -26,7 +26,7 @@ fn check_invariants(url: &Url) {
}


fn run_parsing(input: String, base: String, expected: Result<ExpectedAttributes, ()>) {
fn run_parsing(input: &String, base: &String, expected: Result<ExpectedAttributes, ()>) {

This comment has been minimized.

Sign in to view
@vks

vks May 9, 2017

Why not use &str instead of &String?
@Hoverbear
Hoverbear reviewed May 10, 2017
View changes
src/parser.rs
@@ -1031,7 +1031,7 @@ impl<'a> Parser<'a> {
}
}
None => return Ok((None, None)),
_ => panic!("Programming error. parse_query_and_fragment() called without ? or # {:?}")

This comment has been minimized.

Sign in to view
@Hoverbear

Hoverbear May 10, 2017

Member

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

This comment has been minimized.

Sign in to view
@luisbg

luisbg May 16, 2017

I thought this would give a compiler error. Weird.

This comment has been minimized.

Sign in to view
@Hoverbear

Hoverbear May 17, 2017

Member

Indeed. Me too.

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin May 20, 2017

Member

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
@Hoverbear
Hoverbear approved these changes May 10, 2017
View changes
Copy link
Member

Hoverbear left a comment

LGTM overall. I think @vks's comment about &str should be considered though.
@lucab lucab mentioned this pull request May 10, 2017
Closed
@dtolnay
dtolnay reviewed May 12, 2017
View changes
Copy link

dtolnay left a comment

Ditto @vks, those &String should be &str instead. LGTM otherwise.
@daniellockyer
Remove lifetimes
01ec217
@daniellockyer
Switch double-quotes to single-quotes
844e2d5
@daniellockyer
Use nice helper functions
3c21bd9
@daniellockyer
Tidy up some small nits
6db4c40
@daniellockyer
Updating fuzzing code to latest format
249369a
@daniellockyer
Switch from &String to &str
Loading status checks…
f34fa8f
@daniellockyer
Copy link
Contributor Author

daniellockyer commented May 12, 2017 •
edited

@vks + @Hoverbear + @dtolnay: I've switched it from &String to &str. Good to go?
@vks
Copy link

vks commented May 12, 2017

Great, looks good to me!
@lucab
lucab approved these changes May 12, 2017
View changes
@dtolnay
dtolnay approved these changes May 12, 2017
View changes
@luisbg
Copy link

luisbg commented May 16, 2017

Looks good 😄 These changes make the code nicer.

Except that one change in src/host.rs that is more than pure style cleanup. Not sure if maintainers want that in a new branch or if it is OK as is.

Thanks Daniel
@Hoverbear
Copy link
Member

Hoverbear commented May 17, 2017 •
edited

We have multiple good reviews so lets merge!
@bors-servo
Copy link
Contributor

bors-servo commented May 18, 2017

☔ The latest upstream changes (presumably #342) made this pull request unmergeable. Please resolve the merge conflicts.
@daniellockyer
Merge branch 'master' into clippy-fixes
Loading status checks…
78b4f57
@SimonSapin
Copy link
Member

SimonSapin commented May 20, 2017

@vks I’m not really keeping track of a minimum version. We’ll figure it out when/if someone complains :)

This looks very nice. Thank you all!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2017

📌 Commit 78b4f57 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2017

⌛ Testing commit 78b4f57 with merge e461e34...
bors-servo added a commit that referenced this pull request May 20, 2017
@bors-servo
Auto merge of #294 - neosilky:clippy-fixes, r=SimonSapin
Loading status checks…
e461e34 
Fix up some issues flagged by clippy

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/294)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2017

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing e461e34 to master...
@bors-servo bors-servo merged commit 78b4f57 into servo:master May 20, 2017
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 7 files, 1 discussion left (dtolnay, Hoverbear, lucab, luisbg)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@daniellockyer daniellockyer deleted the daniellockyer:clippy-fixes branch May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vks vks

@Hoverbear Hoverbear

@SimonSapin SimonSapin

@luisbg luisbg

@dtolnay dtolnay

@lucab lucab

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@daniellockyer @vks @luisbg @Hoverbear @bors-servo @SimonSapin @lucab @dtolnay
You can’t perform that action at this time.