Merge pull request #10 from ian-h-chamberlain/cli-improvements

Cli improvements

Author: ian-h-chamberlain

Cargo.lock

@@ -9,4 +9,10 @@ itertools = "0.8.0"
 lazy_static = "1.2.0"
 pest_derive = "2.1.0"
 pest = "2.1.0"
-structopt = "0.2.15"
+structopt = "0.3.5"
+
+[dev-dependencies]
+assert_cmd = "0.11.1"
+predicates = "1.0.2"
+assert_fs = "0.13.1"
+indoc = "0.3.4"

Cargo.toml

@@ -1,33 +1,80 @@
-use std::{error::Error, fs, path::PathBuf};
+use std::{
+    error::Error,
+    fs,
+    io::{self, Read},
+    path::PathBuf,
+};
 
+use lazy_static::lazy_static;
 use structopt::StructOpt;
 
 use crate::parser;
 
+lazy_static! {
+    static ref IO_SENTINEL: PathBuf = PathBuf::from("-");
+}
+
 #[derive(Debug, StructOpt)]
-/// Parse and save Redcode files
+#[structopt(rename_all = "kebab")]
+/// Parse, assemble, and save Redcode files
 struct CliOptions {
-    /// Input file
+    /// Input file; use '-' to read from stdin
     #[structopt(parse(from_os_str))]
     input_file: PathBuf,
 
-    /// Output file; defaults to stdout
-    #[structopt(long, short, parse(from_os_str))]
-    output_file: Option<PathBuf>,
+    #[structopt(subcommand)]
+    command: Command,
+}
+
+#[derive(Debug, StructOpt)]
+enum Command {
+    /// Save/print a program in 'load file' format
+    #[structopt(name = "dump")]
+    Dump {
+        /// Output file; defaults to stdout ('-')
+        #[structopt(long, short, parse(from_os_str), default_value = IO_SENTINEL.to_str().unwrap())]
+        output_file: PathBuf,
+
+        /// Whether labels, expressions, macros, etc. should be resolved and
+        /// expanded in the output
+        #[structopt(long, short = "E")]
+        no_expand: bool,
+    },
 }
 
 pub fn run() -> Result<(), Box<dyn Error>> {
     let cli_options = CliOptions::from_args();
 
-    let input_program = fs::read_to_string(cli_options.input_file)?;
+    let mut input = String::new();
 
-    let parsed_input = parser::parse(input_program.as_str())?;
-    let parse_output = parsed_input.dump();
-
-    if let Some(output_path) = cli_options.output_file {
-        fs::write(output_path, parse_output)?;
+    if cli_options.input_file == *IO_SENTINEL {
+        io::stdin().read_to_string(&mut input)?;
     } else {
-        println!("{}", parse_output);
+        input = fs::read_to_string(cli_options.input_file)?;
+    }
+
+    let mut parsed_core = parser::parse(input.as_str())?;
+
+    // TODO colored output?
+    for warning in parsed_core.warnings.iter() {
+        eprintln!("Warning:\n  {}", warning);
+    }
+
+    match cli_options.command {
+        Command::Dump {
+            output_file,
+            no_expand,
+        } => {
+            if !no_expand {
+                parsed_core.result.resolve()?;
+            }
+
+            if output_file == *IO_SENTINEL {
+                print!("{}", parsed_core);
+            } else {
+                fs::write(output_file, format!("{}", parsed_core))?;
+            };
+        }
     };
 
     Ok(())

src/cli.rs

@@ -4,6 +4,7 @@ extern crate pest_derive;
 
 // Extern crates
 extern crate itertools;
+extern crate lazy_static;
 extern crate pest;
 extern crate structopt;
 

src/lib.rs

@@ -1,14 +1,11 @@
-use std::{
-    collections::{hash_map::Entry, HashMap},
-    fmt,
-};
+use std::{collections::HashMap, fmt};
 
 mod types;
 pub use types::{AddressMode, Modifier, Opcode, Value};
 
 pub const DEFAULT_CORE_SIZE: usize = 8000;
 
-type InstructionSet = Box<[Option<Instruction>]>;
+type Instructions = Box<[Option<Instruction>]>;
 type LabelMap = HashMap<String, usize>;
 
 #[derive(Clone, Debug, PartialEq)]
@@ -109,14 +106,15 @@ impl Instruction {
 }
 
 pub struct Core {
-    instructions: InstructionSet,
+    instructions: Instructions,
+    resolved: Option<Instructions>,
     labels: LabelMap,
 }
 
 impl PartialEq for Core {
-    // TODO: should this impl resolve the instructions? Depends on the use case
     fn eq(&self, other: &Self) -> bool {
-        self.instructions == other.instructions
+        (self.resolved.is_some() && self.resolved == other.resolved)
+            || self.instructions == other.instructions
     }
 }
 
@@ -128,35 +126,35 @@ impl Default for Core {
 
 impl fmt::Debug for Core {
     fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-        write!(
-            formatter,
-            "Labels: {:?}\nCore:\n{}",
-            self.labels,
-            self.dump()
-        )
+        if self.resolved.is_some() {
+            write!(formatter, "{}", self)
+        } else {
+            write!(formatter, "<unresolved core>")
+        }
     }
 }
 
 impl Core {
     pub fn new(core_size: usize) -> Self {
         Core {
             instructions: vec![None; core_size].into_boxed_slice(),
-            labels: HashMap::new(),
+            resolved: None,
+            labels: LabelMap::new(),
         }
     }
 
     pub fn get(&self, index: usize) -> Option<Instruction> {
-        self.instructions.get(index)?.clone()
-    }
-
-    pub fn get_resolved(&self, index: usize) -> Result<Instruction, String> {
-        self.get(index)
-            .unwrap_or_default()
-            .resolve(index, &self.labels)
+        match &self.resolved {
+            Some(instructions) => instructions,
+            None => &self.instructions,
+        }
+        .get(index)?
+        .clone()
     }
 
     pub fn set(&mut self, index: usize, value: Instruction) {
         self.instructions[index] = Some(value);
+        // TODO need to re-resolve here? Or make caller do it
     }
 
     pub fn add_label(&mut self, index: usize, label: String) -> Result<(), String> {
@@ -168,21 +166,19 @@ impl Core {
             ));
         }
 
-        match self.labels.entry(label) {
-            Entry::Occupied(entry) => Err(format!("Label '{}' already exists", entry.key())),
-            Entry::Vacant(entry) => {
-                entry.insert(index);
-                Ok(())
-            }
+        if self.labels.insert(label.clone(), index).is_some() {
+            Err(format!("Label '{}' already exists", label))
+        } else {
+            Ok(())
         }
     }
 
     pub fn label_address(&self, label: &str) -> Option<usize> {
         self.labels.get(label).copied()
     }
 
-    pub fn resolve(&self) -> Result<Self, String> {
-        let instructions = self
+    pub fn resolve(&mut self) -> Result<&mut Self, String> {
+        let resolved = self
             .instructions
             .iter()
             .enumerate()
@@ -192,29 +188,29 @@ impl Core {
                     .map(|instruction| instruction.resolve(i, &self.labels))
                     .transpose()
             })
-            .collect::<Result<InstructionSet, String>>()?;
+            .collect::<Result<_, String>>()?;
 
-        Ok(Self {
-            instructions,
-            ..Default::default()
-        })
+        self.resolved = Some(resolved);
+
+        Ok(self)
     }
+}
 
-    pub fn dump(&self) -> String {
-        // TODO: convert to fmt::Display - this will require some upfront
-        // validation that all labels are valid, etc
-        // It may be desirable to have Debug be a dump() of the load file and
-        // Display show the original parsed document (or something like that)
-        match self.resolve() {
-            Err(msg) => msg,
-            Ok(core) => core
-                .instructions
-                .iter()
-                .filter_map(Option::as_ref)
-                .fold(String::new(), |result, instruction| {
-                    result + &instruction.to_string() + "\n"
-                }),
-        }
+impl fmt::Display for Core {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(
+            f,
+            "{}",
+            match &self.resolved {
+                None => &self.instructions,
+                Some(instructions) => instructions,
+            }
+            .iter()
+            .filter_map(Option::as_ref)
+            .fold(String::new(), |result, instruction| {
+                result + &instruction.to_string() + "\n"
+            })
+        )
     }
 }
 
@@ -251,11 +247,13 @@ mod tests {
         core.add_label(256, "goblin".into())
             .expect_err("Should fail to add labels > 200");
         core.add_label(5, "baz".into())
-            .expect_err("Should fail to add duplicate label");
+            .expect_err("Should error duplicate label");
 
         assert_eq!(core.label_address("foo").unwrap(), 0);
         assert_eq!(core.label_address("bar").unwrap(), 0);
-        assert_eq!(core.label_address("baz").unwrap(), 123);
+
+        // The _last_ version of a label will be the one we use
+        assert_eq!(core.label_address("baz").unwrap(), 5);
 
         assert!(core.label_address("goblin").is_none());
         assert!(core.label_address("never_mentioned").is_none());
@@ -303,12 +301,10 @@ mod tests {
             },
         );
 
-        let resolved_core = core.resolve().expect("Should resolve all labels in core");
+        core.resolve().expect("Should resolve all labels in core");
 
         assert_eq!(
-            resolved_core
-                .get(3)
-                .expect("Should have instruction at pos 5"),
+            core.get(3).expect("Should have instruction at pos 5"),
             Instruction {
                 field_a: Field {
                     value: Value::Literal(4),

src/load_file.rs

@@ -1,9 +1,16 @@
 extern crate corewa_rs;
 
-use std::error::Error;
-
 use corewa_rs::cli;
 
-fn main() -> Result<(), Box<dyn Error>> {
-    cli::run()
+fn main() {
+    std::process::exit(
+        // TODO use exitcode lib or something like that
+        if let Err(err) = cli::run() {
+            eprintln!("Error: {}", err);
+            -1
+        } else {
+            // TODO use exit codes for warnings?
+            0
+        },
+    )
 }