MAINT: Cleanup

TODO and FIXME removals.
Replaced unnecessary alt_peek! macro.
Renamed some types for clarity.

src/ast/mod.rs

@@ -715,11 +715,11 @@ pub struct ListOpDef {
     pub pos: Position,
 }
 
+// TODO(jwall): this should probably be moved to a Val::Module IR type.
 #[derive(Debug, PartialEq, Clone)]
 pub struct ModuleDef {
     pub pos: Position,
     pub arg_set: FieldList,
-    // FIXME(jwall): this should probably be moved to a Val::Module IR type.
     pub arg_tuple: Option<Rc<Val>>,
     pub statements: Vec<Statement>,
 }
@@ -729,7 +729,6 @@ impl ModuleDef {
         ModuleDef {
             pos: pos.into(),
             arg_set: arg_set,
-            // TODO(jwall): Should this get moved to a Val version of our ModuleDef?
             arg_tuple: None,
             statements: stmts,
         }

src/build/mod.rs

@@ -233,7 +233,6 @@ impl<'a> Builder<'a> {
         self.import_stack = new_stack;
     }
 
-    // TOOD(jwall): This needs some unit tests.
     fn tuple_to_val(&mut self, fields: &Vec<(Token, Expression)>) -> Result<Rc<Val>, Box<Error>> {
         let mut new_fields = Vec::<(PositionedItem<String>, Rc<Val>)>::new();
         for &(ref name, ref expr) in fields.iter() {
@@ -434,7 +433,6 @@ impl<'a> Builder<'a> {
     fn eval_let(&mut self, def: &LetDef) -> Result<Rc<Val>, Box<Error>> {
         let val = self.eval_expr(&def.value)?;
         let name = &def.name;
-        // TODO(jwall): Enforce the reserved words list here.
         if Self::check_reserved_word(&name.fragment) {
             return Err(Box::new(error::BuildError::new(
                 format!("Let {} binding collides with reserved word", name.fragment),
@@ -1104,7 +1102,6 @@ impl<'a> Builder<'a> {
         Ok(Rc::new(Val::Str(formatter.render(&def.pos)?)))
     }
 
-    // FIXME(jwall): Handle module calls as well?
     fn eval_call(&mut self, def: &CallDef) -> Result<Rc<Val>, Box<Error>> {
         let sel = &def.macroref;
         let args = &def.arglist;

src/convert/xml.rs

@@ -73,7 +73,6 @@ impl XmlConverter {
         // First we determine if this is a tag or text node
         if let Val::Tuple(ref fs) = v {
             let mut name: Option<&str> = None;
-            // TODO let mut namespace: Option<&str> = None;
             let mut attrs: Option<&Vec<(PositionedItem<String>, Rc<Val>)>> = None;
             let mut children: Option<&Vec<Rc<Val>>> = None;
             let mut text: Option<&str> = None;

src/main.rs

@@ -29,7 +29,6 @@ use ucglib::build::Val;
 use ucglib::convert::traits;
 use ucglib::convert::ConverterRegistry;
 
-// TODO(jwall): List the target output types automatically.
 fn do_flags<'a, 'b>() -> clap::App<'a, 'b> {
     clap_app!(
         ucg =>
@@ -165,7 +164,6 @@ fn visit_ucg_files(
 ) -> Result<bool, Box<Error>> {
     let our_path = String::from(path.to_string_lossy());
     let mut result = true;
-    // TODO(jwall): Report the failing files at the bottom.
     let mut summary = String::new();
     if path.is_dir() {
         let mut dir_iter = std::fs::read_dir(path)?.peekable();

src/parse/mod.rs

@@ -28,15 +28,14 @@ use crate::error::StackPrinter;
 use crate::iter::OffsetStrIter;
 use crate::tokenizer::*;
 
-// TODO(jwall): Rename this to something better.
+type ParseResult<'a, O> = Result<SliceIter<'a, Token>, O>;
-type NomResult<'a, O> = Result<SliceIter<'a, Token>, O>;
 
 #[cfg(feature = "tracing")]
 const ENABLE_TRACE: bool = true;
 #[cfg(not(feature = "tracing"))]
 const ENABLE_TRACE: bool = false;
 
-type ParseResult<'a, O> = std::result::Result<O, abortable_parser::Error<SliceIter<'a, Token>>>;
+type ConvertResult<'a, O> = std::result::Result<O, abortable_parser::Error<SliceIter<'a, Token>>>;
 
 macro_rules! trace_parse {
     ($i:expr, $rule:ident!( $($args:tt)* )) => {
@@ -68,7 +67,7 @@ macro_rules! trace_parse {
     };
 }
 
-fn symbol_to_value(s: &Token) -> ParseResult<Value> {
+fn symbol_to_value(s: &Token) -> ConvertResult<Value> {
     Ok(Value::Symbol(value_node!(
         s.fragment.to_string(),
         s.pos.clone()
@@ -81,7 +80,7 @@ make_fn!(
     match_type!(BAREWORD => symbol_to_value)
 );
 
-fn str_to_value(s: &Token) -> ParseResult<Value> {
+fn str_to_value(s: &Token) -> ConvertResult<Value> {
     Ok(Value::Str(value_node!(
         s.fragment.to_string(),
         s.pos.clone()
@@ -98,7 +97,7 @@ make_fn!(
 fn triple_to_number<'a>(
     input: SliceIter<'a, Token>,
     v: (Option<Token>, Option<Token>, Option<Token>),
-) -> ParseResult<'a, Value> {
+) -> ConvertResult<'a, Value> {
     let (pref, mut pref_pos) = match v.0 {
         None => ("", Position::new(0, 0, 0)),
         Some(ref bs) => (bs.fragment.borrow(), bs.pos.clone()),
@@ -141,94 +140,6 @@ fn triple_to_number<'a>(
     return Ok(Value::Float(value_node!(f, pref_pos)));
 }
 
-// FIXME(jwall): This should actually be unnecessary now.
-
-/// alt_peek conditionally runs a combinator if a lookahead combinator matches.
-macro_rules! alt_peek {
-    (__inner $i:expr, $peekrule:ident!( $($peekargs:tt)* ) => $parserule:ident | $($rest:tt)* ) => (
-        alt_peek!(__inner $i, $peekrule!($($peekargs)*) => run!($parserule) | $($rest)* )
-    );
-
-    (__inner $i:expr, $peekrule:ident => $($rest:tt)* ) => (
-        alt_peek!(__inner $i, run!($peekrule) => $($rest)* )
-    );
-
-    (__inner $i:expr, $peekrule:ident!( $($peekargs:tt)* ) => $parserule:ident!( $($parseargs:tt)* ) | $($rest:tt)* ) => (
-        {
-            let _i = $i.clone();
-            let pre_res = peek!(_i, $peekrule!($($peekargs)*));
-            match pre_res {
-                // if the peek was incomplete then it might still match so return incomplete.
-                Result::Incomplete(i) => Result::Incomplete(i),
-                // If the peek was in error then try the next peek => parse pair.
-                Result::Fail(_) =>  {
-                    alt_peek!(__inner $i, $($rest)*)
-                },
-                Result::Abort(e) => Result::Abort(e),
-                // If the peek was successful then return the result of the parserule
-                // regardless of it's result.
-                Result::Complete(_i, _) => {
-                    $parserule!(_i, $($parseargs)*)
-                },
-            }
-        }
-    );
-
-    // These are our no fallback termination cases.
-    (__inner $i:expr, $peekrule:ident!( $($peekargs:tt)* ) => $parserule:ident, __end ) => (
-        alt_peek!(__inner $i, $peekrule!($($peekargs)*) => call!($parserule), __end )
-    );
-
-    (__inner $i:expr, $peekrule:ident!( $($peekargs:tt)* ) => $parserule:ident!( $($parseargs:tt)* ), __end ) => (
-        {
-            let _i = $i.clone();
-            let pre_res = peek!(_i, $peekrule!($($peekargs)*));
-            match pre_res {
-                // if the peek was incomplete then it might still match so return incomplete.
-                Result::Incomplete(i) => Result::Incomplete(i),
-                // If the peek was in error then try the next peek => parse pair.
-                Result::Fail(_) =>  {
-                    alt_peek!(__inner $i, __end)
-                },
-                Result::Abort(e) => Result::Abort(e),
-                // If the peek was successful then return the result of the parserule
-                // regardless of it's result.
-                Result::Complete(_i, _) => {
-                    $parserule!(_i, $($parseargs)*)
-                },
-            }
-        }
-    );
-
-    // These are our fallback termination cases.
-    (__inner $i:expr, $fallback:ident, __end) => (
-        {
-            let _i = $i.clone();
-            run!(_i, $fallback)
-        }
-    );
-    // In the case of a fallback rule with no peek we just return whatever
-    // the fallback rule returns.
-    (__inner $i:expr, $fallback:ident!( $($args:tt)* ), __end) => (
-        {
-            let _i = $i.clone();
-            $fallback!(_i, $($args)*)
-        }
-    );
-
-    // This is our default termination case.
-    // If there is no fallback then we return an Error.
-    (__inner $i:expr, __end) => {
-        compile_error!("alt_peek! requirs a fallback case");
-    };
-
-    // alt_peek entry_point.
-    ($i:expr, $($rest:tt)*) => {
-        // We use __end to define the termination token the recursive rule should consume.
-        alt_peek!(__inner $i, $($rest)*, __end)
-    };
-}
-
 // trace_macros!(true);
 
 // NOTE(jwall): HERE THERE BE DRAGONS. The order for these matters
@@ -405,7 +316,7 @@ make_fn!(
     )
 );
 
-fn symbol_or_expression(input: SliceIter<Token>) -> NomResult<Expression> {
+fn symbol_or_expression(input: SliceIter<Token>) -> ParseResult<Expression> {
     let _i = input.clone();
     let scalar_head = do_each!(input,
         sym => either!(symbol, compound_value),
@@ -445,7 +356,7 @@ fn symbol_or_expression(input: SliceIter<Token>) -> NomResult<Expression> {
     }
 }
 
-fn selector_list(input: SliceIter<Token>) -> NomResult<SelectorList> {
+fn selector_list(input: SliceIter<Token>) -> ParseResult<SelectorList> {
     let (rest, head) = match symbol_or_expression(input) {
         Result::Complete(rest, val) => (rest, val),
         Result::Fail(e) => {
@@ -529,7 +440,7 @@ fn tuple_to_macro<'a>(
     pos: Position,
     vals: Option<Vec<Value>>,
     val: Value,
-) -> ParseResult<'a, Expression> {
+) -> ConvertResult<'a, Expression> {
     let mut default_args = match vals {
         None => Vec::new(),
         Some(vals) => vals,
@@ -623,7 +534,7 @@ fn tuple_to_select<'a>(
     e1: Expression,
     e2: Expression,
     val: Value,
-) -> ParseResult<'a, Expression> {
+) -> ConvertResult<'a, Expression> {
     match val {
         Value::Tuple(v) => Ok(Expression::Select(SelectDef {
             val: Box::new(e1),
@@ -695,7 +606,7 @@ fn tuple_to_call<'a>(
     input: SliceIter<'a, Token>,
     val: Value,
     exprs: Option<Vec<Expression>>,
-) -> ParseResult<'a, Expression> {
+) -> ConvertResult<'a, Expression> {
     if let Value::Selector(def) = val {
         Ok(Expression::Call(CallDef {
             macroref: def,
@@ -750,7 +661,7 @@ fn tuple_to_list_op<'a>(
     kind: ListOpType,
     macroname: Value,
     list: Expression,
-) -> ParseResult<'a, Expression> {
+) -> ConvertResult<'a, Expression> {
     if let Value::Selector(mut def) = macroname {
         // First of all we need to assert that this is a selector of at least
         // two sections.
@@ -818,7 +729,7 @@ make_fn!(
     )
 );
 
-fn unprefixed_expression(input: SliceIter<Token>) -> NomResult<Expression> {
+fn unprefixed_expression(input: SliceIter<Token>) -> ParseResult<Expression> {
     let _input = input.clone();
     either!(
         input,
@@ -831,16 +742,17 @@ fn unprefixed_expression(input: SliceIter<Token>) -> NomResult<Expression> {
 
 make_fn!(
     non_op_expression<SliceIter<Token>, Expression>,
-    alt_peek!(
+    either!(
-         either!(word!("map"), word!("filter")) => trace_parse!(list_op_expression) |
+        trace_parse!(list_op_expression),
-         word!("macro") => trace_parse!(macro_expression) |
+        trace_parse!(macro_expression),
-         word!("module") => trace_parse!(module_expression) |
+        trace_parse!(module_expression),
-         word!("select") => trace_parse!(select_expression) |
+        trace_parse!(select_expression),
-         punct!("(") => trace_parse!(grouped_expression) |
+        trace_parse!(grouped_expression),
-         trace_parse!(unprefixed_expression))
+        trace_parse!(unprefixed_expression)
+    )
 );
 
-fn expression(input: SliceIter<Token>) -> NomResult<Expression> {
+fn expression(input: SliceIter<Token>) -> ParseResult<Expression> {
     let _input = input.clone();
     match trace_parse!(_input, op_expression) {
         Result::Incomplete(i) => Result::Incomplete(i),
@@ -874,7 +786,6 @@ make_fn!(
     do_each!(
         name => wrap_err!(match_type!(BAREWORD), "Expected name for binding"),
         _ => punct!("="),
-        // TODO(jwall): Wrap this error with an appropriate abortable_parser::Error
         val => wrap_err!(trace_parse!(expression), "Expected Expression"),
         _ => punct!(";"),
         (tuple_to_let(name, val))
@@ -941,11 +852,12 @@ make_fn!(
 
 //trace_macros!(true);
 fn statement(i: SliceIter<Token>) -> Result<SliceIter<Token>, Statement> {
-    return alt_peek!(i,
+    return either!(
-        word!("assert") => trace_parse!(assert_statement) |
+        i,
-        word!("import") => trace_parse!(import_statement) |
+        trace_parse!(assert_statement),
-        word!("let") => trace_parse!(let_statement) |
+        trace_parse!(import_statement),
-        word!("out") => trace_parse!(out_statement) |
+        trace_parse!(let_statement),
+        trace_parse!(out_statement),
         trace_parse!(expression_statement)
     );
 }

src/parse/precedence.rs

@@ -17,7 +17,7 @@
 use abortable_parser::combinators::eoi;
 use abortable_parser::{Error, Result, SliceIter};
 
-use super::{non_op_expression, NomResult};
+use super::{non_op_expression, ParseResult};
 use crate::ast::*;
 
 /// Defines the intermediate stages of our bottom up parser for precedence parsing.
@@ -260,7 +260,7 @@ make_fn!(
 );
 
 /// Parse a list of expressions separated by operators into a Vec<Element>.
-fn parse_operand_list<'a>(i: SliceIter<'a, Token>) -> NomResult<'a, Vec<Element>> {
+fn parse_operand_list<'a>(i: SliceIter<'a, Token>) -> ParseResult<'a, Vec<Element>> {
     // 1. First try to parse a non_op_expression,
     let mut _i = i.clone();
     let mut list = Vec::new();