-
Notifications
You must be signed in to change notification settings - Fork 14k
Cleanup rustc/driver #55008
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
Cleanup rustc/driver #55008
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
Hmm, I'm not sure if this is spurious or not; I'll verify shortly. |
estebank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple of nitpicks.
src/librustc_driver/driver.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering wether the previous version was like that to avoid flushing on every line (I believe that is a behavior difference between write and writeln, although looking around https://llogiq.github.io/2017/06/01/perf-pitfalls.html https://www.reddit.com/r/rust/comments/6hoayo/how_do_i_write_to_stdout_without_line_buffering/ it seems like I might be wrong).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; this change is contained in a single commit, so in case of any issues it can be easily reverted. I can also pull it back if you prefer.
src/librustc_driver/driver.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_else(PathBuf::new);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or unwrap_or_default that I forgot existed ^^.
src/librustc_driver/driver.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work: .unwrap_or_else(input.filestem);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool, but unfortunately the compiler recognizes this as E0615.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's annoying but can see how it happens. Thanks for checking.
src/librustc_driver/lib.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the indentation here, wouldn't the following be better?
let emitter = errors::emitter::EmitterWriter::stderr(
errors::ColorConfig::Auto,
None,
true,
false,
);
src/librustc_driver/pretty.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When encountering these I usually change them to be
fn foo(
&self,
bar,
) -> A {
}
That way we won't have to worry about the function name changing length or adding/removing type args/lifetimes requiring reindenting of the arguments.
|
9c69caa to
b03a82c
Compare
|
Error fixed, comments addressed. |
|
@bors r+ |
|
📌 Commit b03a82c has been approved by |
Cleanup rustc/driver - improve/remove allocations - simplify `profile::trace::cons*` - don't sort `base` if it only has one element - use `Cow<str>` where applicable - use `unwrap_or_else` with function calls - remove an explicit `return`, add an explicit `None` - remove lifetimes from `const`s - improve common patterns - improve macro calls - whitespace & formatting fixes
|
☀️ Test successful - status-appveyor, status-travis |
profile::trace::cons*baseif it only has one elementCow<str>where applicableunwrap_or_elsewith function callsreturn, add an explicitNoneconsts