Cleanup rustc/driver #55008

ljedrz · 2018-10-12T12:39:34Z

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 consts
improve common patterns
improve macro calls
whitespace & formatting fixes

rust-highfive · 2018-10-12T12:39:38Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

ljedrz · 2018-10-12T15:13:43Z

Hmm, I'm not sure if this is spurious or not; I'll verify shortly.

estebank

LGTM, couple of nitpicks.

estebank · 2018-10-12T17:19:30Z

src/librustc_driver/driver.rs

@@ -1472,7 +1473,7 @@ fn write_out_deps(sess: &Session, outputs: &OutputFilenames, out_filenames: &[Pa
            .collect();
        let mut file = fs::File::create(&deps_filename)?;
        for path in out_filenames {
-            write!(file, "{}: {}\n\n", path.display(), files.join(" "))?;
+            writeln!(file, "{}: {}\n", path.display(), files.join(" "))?;


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).

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.

estebank · 2018-10-12T17:20:55Z

src/librustc_driver/driver.rs

-                Some(ref d) => d.clone(),
-                None => PathBuf::new(),
-            };
+            let dirpath = (*odir).as_ref().cloned().unwrap_or_else(|| PathBuf::new());


unwrap_or_else(PathBuf::new);

Or unwrap_or_default that I forgot existed ^^.

estebank · 2018-10-12T17:21:50Z

src/librustc_driver/driver.rs


            // If a crate name is present, we use it as the link name
            let stem = sess.opts
                .crate_name
                .clone()
                .or_else(|| attr::find_crate_name(attrs).map(|n| n.to_string()))
-                .unwrap_or(input.filestem());
+                .unwrap_or_else(|| input.filestem());


I think this should work: .unwrap_or_else(input.filestem);

That would be cool, but unfortunately the compiler recognizes this as E0615.

That's annoying but can see how it happens. Thanks for checking.

estebank · 2018-10-12T17:23:24Z

src/librustc_driver/lib.rs

-                                                                false);
+                                                                   None,
+                                                                   true,
+                                                                   false);


Given the indentation here, wouldn't the following be better?

let emitter = errors::emitter::EmitterWriter::stderr( errors::ColorConfig::Auto, None, true, false, );

estebank · 2018-10-12T17:26:05Z

src/librustc_driver/pretty.rs

+                                            output_filenames: &OutputFilenames,
+                                            id: &str,
+                                            f: F)
+                                            -> A


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.

estebank · 2018-10-12T17:29:54Z

[00:21:43] error: unused attribute
[00:21:43]  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs:5:1
[00:21:43]   |
[00:21:43] 5 | #![crate_type = "rlib"]
[00:21:43]   | ^^^^^^^^^^^^^^^^^^^^^^^
[00:21:43]   |
[00:21:43] note: lint level defined here
[00:21:43]  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs:1:31
[00:21:43]   |
[00:21:43] 1 | #![cfg_attr(not(stage0), deny(warnings))]
[00:21:43]   |                               ^^^^^^^^
[00:21:43]   = note: #[deny(unused_attributes)] implied by #[deny(warnings)]
[00:21:43] 
[00:21:43] error: crate-level attribute should be in the root module
[00:21:43]  --> rustc/compiler_builtins_shim/../../libcompiler_builtins/src/lib.rs:5:1
[00:21:43]   |
[00:21:43] 5 | #![crate_type = "rlib"]
[00:21:43]   | ^^^^^^^^^^^^^^^^^^^^^^^
[00:21:43] 
[00:21:43] error: aborting due to 2 previous errors
[00:21:43] 
[00:21:43] error: Could not compile `compiler_builtins`.

ljedrz · 2018-10-13T08:17:24Z

Error fixed, comments addressed.

estebank · 2018-10-14T23:48:04Z

@bors r+

bors · 2018-10-14T23:48:05Z

📌 Commit b03a82c has been approved by estebank

bors · 2018-10-15T00:18:35Z

⌛ Testing commit b03a82c with merge f02768b...

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

bors · 2018-10-15T02:48:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing f02768b to master...

ljedrz added 2 commits October 12, 2018 13:55

rustc/driver: simplify profile::trace::cons

5bfe08f

rustc/driver: improve macro calls

39753c8

rust-highfive assigned estebank Oct 12, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2018

This comment has been minimized.

Sign in to view

estebank reviewed Oct 12, 2018

View reviewed changes

ljedrz added 8 commits October 13, 2018 10:09

rustc/driver: improve/remove allocations

9a1de08

rustc/driver: use Cow<str> where applicable

2c482d8

rustc/driver: improve common patterns

d599f5b

rustc/driver: remove lifetimes from consts

9b01b51

rustc/driver: don't sort base if it only has one element

292cc1e

rustc/driver: remove explicit return, add explicit None

c46c4d7

rustc/driver: unwrap_or_else with function calls

d838a7f

rustc/driver: whitespace & formatting fixes

b03a82c

ljedrz force-pushed the cleanup_rustc_driver branch from 9c69caa to b03a82c Compare October 13, 2018 08:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2018

bors merged commit b03a82c into rust-lang:master Oct 15, 2018

bors mentioned this pull request Oct 15, 2018

Cleanup rustc/session #54963

Merged

ljedrz deleted the cleanup_rustc_driver branch October 15, 2018 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup rustc/driver #55008

Cleanup rustc/driver #55008

ljedrz commented Oct 12, 2018

rust-highfive commented Oct 12, 2018

This comment has been minimized.

ljedrz commented Oct 12, 2018

estebank left a comment

estebank Oct 12, 2018

ljedrz Oct 13, 2018

estebank Oct 12, 2018

ljedrz Oct 13, 2018

estebank Oct 12, 2018

ljedrz Oct 13, 2018

estebank Oct 14, 2018

estebank Oct 12, 2018

estebank Oct 12, 2018

estebank commented Oct 12, 2018

ljedrz commented Oct 13, 2018

estebank commented Oct 14, 2018

bors commented Oct 14, 2018

bors commented Oct 15, 2018

bors commented Oct 15, 2018

Cleanup rustc/driver #55008

Cleanup rustc/driver #55008

Conversation

ljedrz commented Oct 12, 2018

rust-highfive commented Oct 12, 2018

This comment has been minimized.

ljedrz commented Oct 12, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Oct 12, 2018

ljedrz commented Oct 13, 2018

estebank commented Oct 14, 2018

bors commented Oct 14, 2018

bors commented Oct 15, 2018

bors commented Oct 15, 2018