Skip to content
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

Integrate clippy into Servo; cleanup some of script #7224

Merged
merged 9 commits into from Aug 18, 2015

Conversation

@Manishearth
Copy link
Member

Manishearth commented Aug 15, 2015

The integration is off by default for now. You can try it out with ./mach build --features "script/plugins/clippy".

We're using a branch of clippy with some of the lints changed to Allow, either because they don't apply to us, or because they're noisy and dwarf other warnings (but still should be fixed)

After going through the rest of Servo's warnings I'll figure out which lints we should be keeping.

There's a cargo bug with optional deps that makes it hard for this to work with Cargo.lock -- so this PR contains no changes to lockfiles (and running the build with clippy on may dirty the lockfile, though it gets fixed later)

Review on Reviewable

@Manishearth Manishearth force-pushed the Manishearth:clippy branch 2 times, most recently from 40d2629 to c20f970 Aug 15, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Aug 15, 2015

r? @jdm

@Manishearth
Copy link
Member Author

Manishearth commented Aug 15, 2015

@dzbarsky
Copy link
Member

dzbarsky commented Aug 15, 2015

Reviewed 6 of 6 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: 4 of 21 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/node.rs, line 2091 [r9] (raw file):
this can all be CharacterDataCast::to_ref(self).map(|c| c.Data())


components/script/dom/range.rs, line 341 [r9] (raw file):
Isn't this just bp_position(....) == Ordering::Greater && bp_position(...) == Ordering::Less ?


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member Author

Manishearth commented Aug 15, 2015

The single_match lint probably could be improved to suggest using if instead of if let when we're matching against unit variants which impl PartialEq.

(I just blindly copied those changes from Clippy's output, verifying that they were correct, but not thinking more about it 😄 )

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 15, 2015

Review status: 3 of 21 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/range.rs, line 341 [r10] (raw file):
No need for an if at all here.


components/script/dom/treewalker.rs, line 195 [r10] (raw file):
I really really don't like if let constant = ....


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member Author

Manishearth commented Aug 15, 2015

Review status: 3 of 21 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/script/dom/range.rs, line 341 [r10] (raw file):
Ironically, clippy caught this too (and suggested removing the if), just that I didn't notice it :P


components/script/dom/treewalker.rs, line 195 [r10] (raw file):
Fixing

(We're tweaking clippy to suggest if whenever possible, too)


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2015

The latest upstream changes (presumably #7006) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth Manishearth force-pushed the Manishearth:clippy branch from 7e8103d to 718ad12 Aug 15, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Aug 16, 2015

The latest upstream changes (presumably #7236) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth Manishearth force-pushed the Manishearth:clippy branch from 718ad12 to 603e51c Aug 18, 2015
@Manishearth Manishearth force-pushed the Manishearth:clippy branch from 603e51c to 77b3c04 Aug 18, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 18, 2015

Reviewed 1 of 6 files at r1, 2 of 2 files at r2, 1 of 2 files at r6, 1 of 3 files at r8, 9 of 11 files at r9, 11 of 11 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/node.rs, line 278 [r11] (raw file):
Uh, where'd the & go?


Comments from the review on Reviewable.io

@Manishearth Manishearth force-pushed the Manishearth:clippy branch from 77b3c04 to 83a188a Aug 18, 2015
@Ms2ger Ms2ger unassigned jdm Aug 18, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 18, 2015

-S-awaiting-review -S-needs-rebase +S-needs-squash


Reviewed 1 of 1 files at r12.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

… URL, VirtualMethods
@Manishearth Manishearth force-pushed the Manishearth:clippy branch from 83a188a to 19241c9 Aug 18, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2015

@bors-servo r=Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

📌 Commit 0000000 has been approved by Ms2ger

@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2015

erwha

@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2015

@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2015

@bors-servo r=Ms2ger 19241c9

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

🙀 19241c9 is not a valid commit SHA. Please try again with 0000000.

@Manishearth Manishearth reopened this Aug 18, 2015
@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2015

@bors-servo r=Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

📌 Commit 19241c9 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

Testing commit 19241c9 with merge 50e1c96...

bors-servo pushed a commit that referenced this pull request Aug 18, 2015
Integrate clippy into Servo; cleanup some of script

The integration is off by default for now. You can try it out with `./mach build --features "script/plugins/clippy"`.

We're using a branch of clippy with some of the lints changed to Allow, either because they don't apply to us, or because they're noisy and dwarf other warnings (but still should be fixed)

After going through the rest of Servo's warnings I'll figure out which lints we should be keeping.

There's a cargo bug with optional deps that makes it hard for this to work with Cargo.lock -- so this PR contains no changes to lockfiles (and running the build with clippy on may dirty the lockfile, though it gets fixed later)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7224)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

@bors-servo bors-servo merged commit 19241c9 into servo:master Aug 18, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth Manishearth deleted the Manishearth:clippy branch Sep 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.