From 286cb9dd5950ec661ec5c755407c79adb2ffa068 Mon Sep 17 00:00:00 2001 From: Agustin Chiappe Berrini Date: Wed, 13 Dec 2017 05:18:06 -0500 Subject: [PATCH] writeln!(stderr, ...) -> eprintln! While working in other tasks, I noticed that there's a rather inconsistent way to handle errors. In some places we use `eprintln!` and in others `writeln!` to standard error. It seems like the use of `writeln!` don't have an strong motive, as the output is always to standard error, the lock is always handled the same than `eprintln` and there's no obvious (to me) optimization, so I decided to use the more idiomatic `eprintln!` everywhere. --- src/builtins/job_control.rs | 12 +-------- src/builtins/mod.rs | 24 +++++------------- src/builtins/variables.rs | 49 ++++++++----------------------------- src/shell/binary/readln.rs | 4 +-- src/shell/flow.rs | 42 +++++++++---------------------- src/shell/mod.rs | 2 +- 6 files changed, 30 insertions(+), 103 deletions(-) diff --git a/src/builtins/job_control.rs b/src/builtins/job_control.rs index 7d2faac36..9b470d9f2 100644 --- a/src/builtins/job_control.rs +++ b/src/builtins/job_control.rs @@ -5,7 +5,6 @@ use shell::Shell; use shell::job_control::{JobControl, ProcessState}; use shell::signals; use shell::status::*; -use std::io::{stderr, Write}; /// Disowns given process job IDs, and optionally marks jobs to not receive SIGHUP signals. /// The `-a` flag selects all jobs, `-r` selects all running jobs, and `-h` specifies to mark @@ -79,18 +78,9 @@ pub(crate) fn disown(shell: &mut Shell, args: &[&str]) -> Result<(), String> { /// Display a list of all jobs running in the background. pub(crate) fn jobs(shell: &mut Shell) { - let stderr = stderr(); - let mut stderr = stderr.lock(); for (id, process) in shell.background.lock().unwrap().iter().enumerate() { if process.state != ProcessState::Empty { - let _ = writeln!( - stderr, - "[{}] {} {}\t{}", - id, - process.pid, - process.state, - process.name - ); + eprintln!("[{}] {} {}\t{}", id, process.pid, process.state, process.name); } } } diff --git a/src/builtins/mod.rs b/src/builtins/mod.rs index dafbbbc95..8c254560e 100644 --- a/src/builtins/mod.rs +++ b/src/builtins/mod.rs @@ -303,9 +303,7 @@ fn builtin_eval(args: &[&str], shell: &mut Shell) -> i32 { shell.on_command(&buffer.consume()); shell.previous_status } else { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "ion: supplied eval expression was not terminted"); + eprintln!("ion: supplied eval expression was not terminted"); FAILURE } } @@ -355,9 +353,7 @@ fn builtin_test(args: &[&str], _: &mut Shell) -> i32 { Ok(true) => SUCCESS, Ok(false) => FAILURE, Err(why) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); FAILURE } } @@ -368,9 +364,7 @@ fn builtin_calc(args: &[&str], _: &mut Shell) -> i32 { match calc::calc(&args[1..]) { Ok(()) => SUCCESS, Err(why) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); FAILURE } } @@ -383,9 +377,7 @@ fn builtin_random(args: &[&str], _: &mut Shell) -> i32 { match random::random(&args[1..]) { Ok(()) => SUCCESS, Err(why) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); FAILURE } } @@ -447,9 +439,7 @@ fn builtin_disown(args: &[&str], shell: &mut Shell) -> i32 { match job_control::disown(shell, &args[1..]) { Ok(()) => SUCCESS, Err(err) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "ion: disown: {}", err); + eprintln!("ion: disown: {}", err); FAILURE } } @@ -585,9 +575,7 @@ fn builtin_exists(args: &[&str], shell: &mut Shell) -> i32 { Ok(true) => SUCCESS, Ok(false) => FAILURE, Err(why) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); FAILURE } } diff --git a/src/builtins/variables.rs b/src/builtins/variables.rs index 289c011f4..e58970f06 100644 --- a/src/builtins/variables.rs +++ b/src/builtins/variables.rs @@ -115,8 +115,7 @@ fn parse_alias(args: &str) -> Binding { pub(crate) fn alias(vars: &mut Variables, args: &str) -> i32 { match parse_alias(args) { Binding::InvalidKey(key) => { - let stderr = io::stderr(); - let _ = writeln!(&mut stderr.lock(), "ion: alias name, '{}', is invalid", key); + eprintln!("ion: alias name, '{}', is invalid", key); return FAILURE; } Binding::KeyValue(key, value) => { @@ -124,17 +123,11 @@ pub(crate) fn alias(vars: &mut Variables, args: &str) -> i32 { } Binding::ListEntries => print_list(&vars.aliases), Binding::KeyOnly(key) => { - let stderr = io::stderr(); - let _ = writeln!( - &mut stderr.lock(), - "ion: please provide value for alias '{}'", - key - ); + eprintln!("ion: please provide value for alias '{}'", key); return FAILURE; } _ => { - let stderr = io::stderr(); - let _ = writeln!(&mut stderr.lock(), "ion: invalid alias syntax"); + eprintln!("ion: invalid alias syntax"); return FAILURE; } } @@ -149,18 +142,12 @@ where { let args = args.into_iter().collect::>(); if args.len() <= 1 { - let stderr = io::stderr(); - let _ = writeln!(&mut stderr.lock(), "ion: you must specify an alias name"); + eprintln!("ion: you must specify an alias name"); return FAILURE; } for alias in args.iter().skip(1) { if vars.aliases.remove(alias.as_ref()).is_none() { - let stderr = io::stderr(); - let _ = writeln!( - &mut stderr.lock(), - "ion: undefined alias: {}", - alias.as_ref() - ); + eprintln!("ion: undefined alias: {}", alias.as_ref()); return FAILURE; } } @@ -174,28 +161,18 @@ where { let args = args.into_iter().collect::>(); if args.len() <= 2 { - let stderr = io::stderr(); - let _ = writeln!(&mut stderr.lock(), "ion: you must specify an array name"); + eprintln!("ion: you must specify an array name"); return FAILURE; } if args[1].as_ref() != "-a" { - let stderr = io::stderr(); - let _ = writeln!( - &mut stderr.lock(), - "ion: drop_array must be used with -a option" - ); + eprintln!("ion: drop_array must be used with -a option"); return FAILURE; } for array in args.iter().skip(2) { if vars.unset_array(array.as_ref()).is_none() { - let stderr = io::stderr(); - let _ = writeln!( - &mut stderr.lock(), - "ion: undefined array: {}", - array.as_ref() - ); + eprintln!("ion: undefined array: {}", array.as_ref()); return FAILURE; } } @@ -209,19 +186,13 @@ where { let args = args.into_iter().collect::>(); if args.len() <= 1 { - let stderr = io::stderr(); - let _ = writeln!(&mut stderr.lock(), "ion: you must specify a variable name"); + eprintln!("ion: you must specify a variable name"); return FAILURE; } for variable in args.iter().skip(1) { if vars.unset_var(variable.as_ref()).is_none() { - let stderr = io::stderr(); - let _ = writeln!( - &mut stderr.lock(), - "ion: undefined variable: {}", - variable.as_ref() - ); + eprintln!("ion: undefined variable: {}", variable.as_ref()); return FAILURE; } } diff --git a/src/shell/binary/readln.rs b/src/shell/binary/readln.rs index dae0c5d86..8f6f2899d 100644 --- a/src/shell/binary/readln.rs +++ b/src/shell/binary/readln.rs @@ -126,9 +126,7 @@ pub(crate) fn readln(shell: &mut Shell) -> Option { // Handles Ctrl + D Err(ref err) if err.kind() == ErrorKind::UnexpectedEof => break, Err(err) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "ion: liner: {}", err); + eprintln!("ion: liner: {}", err); return None; } } diff --git a/src/shell/flow.rs b/src/shell/flow.rs index 301b458fa..881499fda 100644 --- a/src/shell/flow.rs +++ b/src/shell/flow.rs @@ -8,7 +8,7 @@ use parser::{expand_string, parse_and_validate, ForExpression, StatementSplitter use parser::assignments::{is_array, ReturnValue}; use parser::pipelines::Pipeline; use shell::assignments::VariableStore; -use std::io::{self, stdout, Write}; +use std::io::{stdout, Write}; use std::iter; use std::mem; use types::Array; @@ -84,9 +84,7 @@ impl FlowLogic for Shell { // statement in memory if needed. We can tell if there is a partial statement // later if the value of `level` is not set to `0`. if let Err(why) = self.execute_toplevel(&mut iterator, statement) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); self.flow_control.level = 0; self.flow_control.current_if_mode = 0; return; @@ -127,18 +125,14 @@ impl FlowLogic for Shell { ) { Ok(mode) => mode, Err(why) => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); 4 } }; } &mut Statement::Match { ref mut cases, .. } => { if let Err(why) = collect_cases(&mut iterator, cases, level) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); } } &mut Statement::Time(ref mut box_stmt) => { @@ -257,9 +251,7 @@ impl FlowLogic for Shell { // Capture any leftover statements. while let Some(statement) = iterator.next() { if let Err(why) = self.execute_toplevel(&mut iterator, statement) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); self.flow_control.level = 0; self.flow_control.current_if_mode = 0; return; @@ -430,9 +422,7 @@ impl FlowLogic for Shell { &mut self.flow_control.level, 0, ) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); self.flow_control.level = 0; self.flow_control.current_if_mode = 0; return Condition::Break; @@ -505,9 +495,7 @@ impl FlowLogic for Shell { if let Err(why) = collect_cases(&mut iterator, &mut cases, &mut self.flow_control.level) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); self.flow_control.level = 0; self.flow_control.current_if_mode = 0; return Condition::Break; @@ -764,9 +752,7 @@ impl FlowLogic for Shell { let time = ::std::time::Instant::now(); if let Err(why) = self.execute_toplevel(iterator, *box_statement) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); self.flow_control.level = 0; self.flow_control.current_if_mode = 0; } @@ -799,15 +785,11 @@ impl FlowLogic for Shell { } // At this level, else and else if keywords are forbidden. Statement::ElseIf { .. } | Statement::Else => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "ion: syntax error: not an if statement"); + eprintln!("ion: syntax error: not an if statement"); } // Likewise to else and else if, the end keyword does nothing here. Statement::End => { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "ion: syntax error: no block to end"); + eprintln!("ion: syntax error: no block to end"); } // Collect all cases that are being used by a match construct Statement::Match { @@ -817,9 +799,7 @@ impl FlowLogic for Shell { self.flow_control.level += 1; if let Err(why) = collect_cases(iterator, &mut cases, &mut self.flow_control.level) { - let stderr = io::stderr(); - let mut stderr = stderr.lock(); - let _ = writeln!(stderr, "{}", why); + eprintln!("{}", why); } if self.flow_control.level == 0 { // If all blocks were read we execute the statement diff --git a/src/shell/mod.rs b/src/shell/mod.rs index b76ab0ce3..6d8df0769 100644 --- a/src/shell/mod.rs +++ b/src/shell/mod.rs @@ -39,7 +39,7 @@ use parser::{ArgumentSplitter, Expander, Select}; use smallvec::SmallVec; use std::env; use std::fs::File; -use std::io::{self, Read, Write}; +use std::io::{self, Read}; use std::iter::FromIterator; use std::ops::Deref; use std::path::Path;