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

Misc rust fixes #5982

Merged
merged 1 commit into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 19, 2018

These were mostly suggested by clippy, and are grouped in commits by different suggestions.

@illicitonion illicitonion requested a review from stuhood Jun 19, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -387,7 +387,7 @@ impl Select {
&[
externs::store_bytes(&result.0.stdout),
externs::store_bytes(&result.0.stderr),
externs::store_i64(result.0.exit_code as i64),
externs::store_i64(i64::from(result.0.exit_code)),

This comment has been minimized.

@stuhood

stuhood Jun 19, 2018

Member

Could this be result.0.exit_code.into()?

This comment has been minimized.

@illicitonion

illicitonion Jun 19, 2018

Contributor

It could, and now it is!

@@ -679,7 +679,7 @@ impl RuleGraph {
})
.collect::<Vec<String>>();
internal_rule_strs.sort();
write!(f, "{}\n", internal_rule_strs.join("\n"))?;
writeln!(f, "{}", internal_rule_strs.join("\n"))?;
write!(f, "}}")

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 19, 2018

Contributor

Maybe this too, since I don't think an ending newline matters, and the consistency would be good.

This comment has been minimized.

@illicitonion

illicitonion Jun 19, 2018

Contributor

Done :)

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 19, 2018

So, thinking about this more, the timing of this change is unfortunate. Landing this will basically guarantee that no rust code is cherry-pickable between master and 1.8.x (unless we also cherry pick this).

Cherry-picking this would probably be very low risk (...it really is crazy how good the compiler is), but if this branch is relatively easy to recreate, it would be awesome to wait a week before landing it. If not, I guess landing it and gambling on whether we need to cherry pick it would be ok. I'm probably fine either way: up to you.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jun 19, 2018

Happy to hold off; the changes are pretty much all one-liners, so should be easy to rebase :)

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 22, 2018

@illicitonion : I think you're good to land this one at this point: I don't have explicit plans to cherry-pick any rust fixes.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 27, 2018

@illicitonion : If you're around to rebase this in the next few hours, I'll wait to land #6035 until it is in... otherwise, I'll probably land #6035 since it is green. Sorry!

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/clippy/all branch from eb56dc5 to 8a85330 Jun 27, 2018

@illicitonion illicitonion merged commit 4417517 into pantsbuild:master Jun 27, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/clippy/all branch Jun 27, 2018

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