Skip to content

Fix clippy warnings - #1890

Merged
kinnison merged 13 commits into
rust-lang:masterfrom
tesuji:fix-clippy
Jun 24, 2019
Merged

Fix clippy warnings#1890
kinnison merged 13 commits into
rust-lang:masterfrom
tesuji:fix-clippy

Conversation

@tesuji

@tesujitesuji commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

Note: Please squash these commits before merge.

@rbtcollins

rbtcollins commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

I have a large branch that will conflict, I think it will be easier for you to rebase and eject a couple of your clippy fixes than the other way around - please hold off on merging a couple days.

@bors

bors commented Jun 15, 2019

Copy link
Copy Markdown
Contributor

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

@kinnison

kinnison commented Jun 19, 2019

Copy link
Copy Markdown
Contributor

@lzutao Could you please rebase, and resubmit?

@tesuji

tesuji commented Jun 20, 2019

Copy link
Copy Markdown
ContributorAuthor

Sorry for the delay! I pushed new changes.

kinnison
kinnison suggested changes Jun 21, 2019
View reviewed changes

@kinnisonkinnison left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly looks sane. I only commented on the first Typename -> Self conversion, but I'd like to know which lint is making that desirable because to me it's less readable in some cases, though better in others.

A suggestion is present for some of your refactoring, otherwise looks good.

Comment threadsrc/config.rs
let dist_root = dist_root_server.clone() + "/dist";

let cfg = Cfg {
let cfg = Self {

@kinnisonkinnisonJun 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a new lint I was previously unaware of?

@tesujitesujiJun 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I just enabled the lint in this PR.

@kinnisonkinnisonJun 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aah I see. This one is a pedantic lint and I'm not sure I completely care for it. I won't say "no" this time, but it feels a bit overly pedantic for my liking. Can make it harder to tell what something is if it's overused IMO.

Comment threadsrc/diskio/threaded.rs Outdated
Comment threadsrc/diskio/threaded.rs
Comment threadsrc/diskio/threaded.rs
@tesuji
tesujiforce-pushed the fix-clippy branch 2 times, most recently from 204ebbc to 408f85bCompareJune 21, 2019 12:14
@tesuji

tesuji commented Jun 21, 2019

Copy link
Copy Markdown
ContributorAuthor

Honestly, I don't know how handler and code around that work
so I am afraid of side effect that handler could cause. But if you
are OK with this, I change it now.

@kinnison

kinnison commented Jun 21, 2019

Copy link
Copy Markdown
Contributor

The changes look good, but the build failed. And differently this time -- the dist code simply stopped by the looks of it.

@tesuji

tesuji commented Jun 22, 2019

Copy link
Copy Markdown
ContributorAuthor

The dist test is passed on my Azure build.

@kinnison
kinnison merged commit 2ece700 into rust-lang:masterJun 24, 2019
@borsbors mentioned this pull request Jun 24, 2019
Merged
@tesuji
tesuji deleted the fix-clippy branch June 24, 2019 10:49
@tesuji

tesuji commented Jun 24, 2019

Copy link
Copy Markdown
ContributorAuthor

Oops! You forgot to squash this PR before merging.

@kinnison

kinnison commented Jun 25, 2019

Copy link
Copy Markdown
Contributor

Oops! You forgot to squash this PR before merging.

I didn't really see a need to squash it. Each commit was clear what it was doing.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@kinnisonkinnisonkinnison requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tesuji@rbtcollins@bors@kinnison