From b756a57345a4b303d87d4d2b3f2fd2ab33f6d93c Mon Sep 17 00:00:00 2001 From: Keunwoo Lee Date: Sun, 25 Jan 2015 22:02:48 -0800 Subject: [PATCH 1/4] od: clean up parse_radix code and use site + Make parse_radix terser and clearer. + Make purpose of radix clearer at use site (note that the code currently completely misuses the --address-radix flag; this is inherited from the previous code) + Don't panic! inside parse_radix; instead return Result<> and let the caller handle errors (currently we panic, but probably we'll want to use some less alarming error routine); this will be more testable later as well. --- src/od/od.rs | 53 ++++++++++++++++++++++------------------------------ 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/od/od.rs b/src/od/od.rs index fca7f2962..8c2b0cb16 100644 --- a/src/od/od.rs +++ b/src/od/od.rs @@ -46,13 +46,10 @@ pub fn uumain(args: Vec) -> isize { Err(f) => panic!("Invalid options\n{}", f) }; - - let mut rad = Radix::Octal; - if matches.opt_present("A") { - rad = parse_radix(matches.opt_str("A")); - } else { - println!("{}", getopts::usage("od", &opts)); - } + let input_offset_base = match parse_radix(matches.opt_str("A")) { + Ok(r) => r, + Err(f) => { panic!("Invalid -A/--address-radix\n{}", f) } + }; let mut fname; match args.last() { @@ -60,12 +57,12 @@ pub fn uumain(args: Vec) -> isize { None => { panic!("Need fname for now") ; } }; - main(rad, fname.clone()); + main(input_offset_base, fname.clone()); 0 } -fn main(radix: Radix, fname: String) { +fn main(input_offset_base: Radix, fname: String) { let mut f = match File::open(&Path::new(fname)) { Ok(f) => f, Err(e) => panic!("file error: {}", e) @@ -77,7 +74,7 @@ fn main(radix: Radix, fname: String) { match f.read(bytes) { Ok(n) => { print!("{:07o}", addr); - match radix { + match input_offset_base { Radix::Decimal => {}, Radix::Octal => { for b in range(0, n / std::u16::BYTES) { @@ -99,30 +96,24 @@ fn main(radix: Radix, fname: String) { }; } -fn parse_radix(radix_str: Option) -> Radix { - let rad = match radix_str { +fn parse_radix(radix_str: Option) -> Result { + match radix_str { + None => Ok(Radix::Octal), Some(s) => { let st = s.into_bytes(); if st.len() != 1 { - panic!("Radix must be one of [d, o, b, x]\n"); - } - - let radix: char = *(st.get(0) - .expect("byte string of length 1 lacks a 0th elem")) as char; - if radix == 'd' { - Radix::Decimal - } else if radix == 'x' { - Radix::Hexadecimal - } else if radix == 'o' { - Radix::Octal - } else if radix == 'b' { - Radix::Binary + Err("Radix must be one of [d, o, b, x]\n") } else { - panic!("Radix must be one of [d, o, b, x]\n"); + let radix: char = *(st.get(0) + .expect("byte string of length 1 lacks a 0th elem")) as char; + match radix { + 'd' => Ok(Radix::Decimal), + 'x' => Ok(Radix::Hexadecimal), + 'o' => Ok(Radix::Octal), + 'b' => Ok(Radix::Binary), + _ => Err("Radix must be one of [d, o, b, x]\n") + } } - }, - None => Radix::Octal - }; - - rad + } + } } From 71bc68a987327aa22d8dff4e15f58c8bb2d6d7fb Mon Sep 17 00:00:00 2001 From: Keunwoo Lee Date: Sun, 25 Jan 2015 22:11:33 -0800 Subject: [PATCH 2/4] od: rm -v alias for --version This shortflag conflicts with --output-duplicates. --- src/od/od.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/od/od.rs b/src/od/od.rs index 8c2b0cb16..58c61416e 100644 --- a/src/od/od.rs +++ b/src/od/od.rs @@ -38,7 +38,7 @@ pub fn uumain(args: Vec) -> isize { specified."), "BYTES"), getopts::optflag("h", "help", "display this help and exit."), - getopts::optflag("v", "version", "output version information and exit."), + getopts::optflag("", "version", "output version information and exit."), ]; let matches = match getopts::getopts(args.tail(), &opts) { From 55be34a2c4de46d535b443b56dcd72c1e4f752fb Mon Sep 17 00:00:00 2001 From: Keunwoo Lee Date: Sun, 25 Jan 2015 22:21:38 -0800 Subject: [PATCH 3/4] od: clean up fname usage + rm superfluous clone + rm superfluous mut variable --- src/od/od.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/od/od.rs b/src/od/od.rs index 58c61416e..6766dc664 100644 --- a/src/od/od.rs +++ b/src/od/od.rs @@ -51,18 +51,17 @@ pub fn uumain(args: Vec) -> isize { Err(f) => { panic!("Invalid -A/--address-radix\n{}", f) } }; - let mut fname; - match args.last() { - Some(n) => fname = n, - None => { panic!("Need fname for now") ; } + let fname = match args.last() { + Some(n) => n, + None => { panic!("Need fname for now") ; } }; - main(input_offset_base, fname.clone()); + main(input_offset_base, fname.as_slice()); 0 } -fn main(input_offset_base: Radix, fname: String) { +fn main(input_offset_base: Radix, fname: &str) { let mut f = match File::open(&Path::new(fname)) { Ok(f) => f, Err(e) => panic!("file error: {}", e) From f6a07663e8ebe13b5703e24c9c127c2941e2c36c Mon Sep 17 00:00:00 2001 From: Keunwoo Lee Date: Sun, 25 Jan 2015 22:39:49 -0800 Subject: [PATCH 4/4] od: proper interpretation of -A flag Prior to this CL, --address-radix was being used to determine the format of the output bytes. This was wrong: this flag controls the printing of the address (in the POSIX spec for od, this is called the "input offset base"), not the printing of the content bytes. --- src/od/od.rs | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/od/od.rs b/src/od/od.rs index 6766dc664..d20667fdc 100644 --- a/src/od/od.rs +++ b/src/od/od.rs @@ -56,12 +56,12 @@ pub fn uumain(args: Vec) -> isize { None => { panic!("Need fname for now") ; } }; - main(input_offset_base, fname.as_slice()); + main(&input_offset_base, fname.as_slice()); 0 } -fn main(input_offset_base: Radix, fname: &str) { +fn main(input_offset_base: &Radix, fname: &str) { let mut f = match File::open(&Path::new(fname)) { Ok(f) => f, Err(e) => panic!("file error: {}", e) @@ -72,25 +72,22 @@ fn main(input_offset_base: Radix, fname: &str) { loop { match f.read(bytes) { Ok(n) => { - print!("{:07o}", addr); - match input_offset_base { - Radix::Decimal => {}, - Radix::Octal => { - for b in range(0, n / std::u16::BYTES) { - let bs = &bytes[(2 * b) .. (2 * b + 2)]; - let p: u16 = (bs[1] as u16) << 8 | bs[0] as u16; - print!(" {:06o}", p); - } - if n % std::u16::BYTES == 1 { - print!(" {:06o}", bytes[n - 1]); - } - } - _ => { } - }; + print_with_radix(input_offset_base, addr); + for b in range(0, n / std::u16::BYTES) { + let bs = &bytes[(2 * b) .. (2 * b + 2)]; + let p: u16 = (bs[1] as u16) << 8 | bs[0] as u16; + print!(" {:06o}", p); + } + if n % std::u16::BYTES == 1 { + print!(" {:06o}", bytes[n - 1]); + } print!("\n"); addr += n; }, - Err(_) => { println!("{:07o}", addr); break; } + Err(_) => { + print_with_radix(input_offset_base, addr); + break; + } }; }; } @@ -116,3 +113,14 @@ fn parse_radix(radix_str: Option) -> Result { } } } + +fn print_with_radix(r: &Radix, x: usize) { + // TODO(keunwoo): field widths should be based on sizeof(x), or chosen dynamically based on the + // expected range of address values. Binary in particular is not great here. + match *r { + Radix::Decimal => print!("{:07}", x), + Radix::Hexadecimal => print!("{:07X}", x), + Radix::Octal => print!("{:07o}", x), + Radix::Binary => print!("{:07b}", x) + } +}