Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
use PAGER to view --explain output #32665
  • Loading branch information
cengiz-io committed Jul 3, 2017
commit 3146e552d199a4db223773e0b80ce8cd510df78a
46 changes: 43 additions & 3 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,11 @@ use std::cmp::max;
use std::cmp::Ordering::Equal;
use std::default::Default;
use std::env;
use std::ffi::OsString;
use std::io::{self, Read, Write};
use std::iter::repeat;
use std::path::PathBuf;
use std::process;
use std::process::{self, Command, Stdio};
use std::rc::Rc;
use std::str;
use std::sync::{Arc, Mutex};
Expand Down Expand Up @@ -354,27 +355,66 @@ fn handle_explain(code: &str,
match descriptions.find_description(&normalised) {
Some(ref description) => {
let mut is_in_code_block = false;
let mut text = String::new();

// Slice off the leading newline and print.
for line in description[1..].lines() {
let indent_level = line.find(|c: char| !c.is_whitespace())
.unwrap_or_else(|| line.len());
let dedented_line = &line[indent_level..];
if dedented_line.starts_with("```") {
is_in_code_block = !is_in_code_block;
println!("{}", &line[..(indent_level+3)]);
text.push_str(&line[..(indent_level+3)]);
text.push('\n');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carols10cents maybe this can be prettier. Suggestions?

Copy link

Choose a reason for hiding this comment

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

IMO It's ok to do that push(). Though you are doing it here and in like 375, so you are adding a carriage return regardless of the if statement, I would just put a single one between lines 376-377 instead, but that is totally a nitpick.

} else if is_in_code_block && dedented_line.starts_with("# ") {
continue;
} else {
println!("{}", line);
text.push_str(line);
text.push('\n');
}
}

show_content_with_pager(&text);
}
None => {
early_error(output, &format!("no extended information for {}", code));
}
}
}

fn show_content_with_pager(content: &String) {
let pager_name = env::var_os("PAGER").unwrap_or(if cfg!(windows) {
OsString::from("more.com")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum I hope this is the right way of using OsStrings

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so, but unwrap_or_else please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need a unwrap_or_else here?

Copy link
Member

Choose a reason for hiding this comment

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

OsString is heap allocated, and with unwrap_or the heap allocation always happens even if not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense now. Added!

} else {
OsString::from("less")
});

let mut fallback_to_println = false;

match Command::new(pager_name).stdin(Stdio::piped()).spawn() {
Ok(mut pager) => {
if let Some(mut pipe) = pager.stdin.as_mut() {
if pipe.write_all(content.as_bytes()).is_err() {
fallback_to_println = true;
}
}

if pager.wait().is_err() {
fallback_to_println = true;
}
}
Err(_) => {
fallback_to_println = true;
}
}

// If pager fails for whatever reason, we should still print the content
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum as you suggested, now we're falling back to plain printing whenever something goes wrong. And not printing anything to stdout if there's an error.

// to standard output
if fallback_to_println {
println!("{}", content);
Copy link
Member

@kennytm kennytm Jun 29, 2017

Choose a reason for hiding this comment

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

UI test failure. Please either change this to print! or update src/test/ui/explain.stdout to include the new trailing \n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennytm thanks again.

}
}

impl<'a> CompilerCalls<'a> for RustcDefaultCalls {
fn early_callback(&mut self,
matches: &getopts::Matches,
Expand Down