From 821f1e9fb22feb4e3c222ff1fec9e74fd55651ae Mon Sep 17 00:00:00 2001 From: Jeremy Wall Date: Fri, 14 Dec 2018 16:01:17 -0600 Subject: [PATCH] MAINT: Cleanup TODO and FIXME removals. Replaced unnecessary alt_peek! macro. Renamed some types for clarity. --- src/ast/mod.rs | 3 +- src/build/mod.rs | 3 - src/convert/xml.rs | 1 - src/main.rs | 2 - src/parse/mod.rs | 142 ++++++++-------------------------------- src/parse/precedence.rs | 4 +- 6 files changed, 30 insertions(+), 125 deletions(-) diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 04fd0d3..6c3222f 100644 --- a/src/ast/mod.rs +++ b/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>, pub statements: Vec, } @@ -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, } diff --git a/src/build/mod.rs b/src/build/mod.rs index 6d7c673..2440808 100644 --- a/src/build/mod.rs +++ b/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, Box> { let mut new_fields = Vec::<(PositionedItem, Rc)>::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, Box> { 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, Box> { let sel = &def.macroref; let args = &def.arglist; diff --git a/src/convert/xml.rs b/src/convert/xml.rs index 0b40938..bcc4bc7 100644 --- a/src/convert/xml.rs +++ b/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, Rc)>> = None; let mut children: Option<&Vec>> = None; let mut text: Option<&str> = None; diff --git a/src/main.rs b/src/main.rs index c0bef20..f32c476 100644 --- a/src/main.rs +++ b/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> { 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(); diff --git a/src/parse/mod.rs b/src/parse/mod.rs index 37e8b91..f666744 100644 --- a/src/parse/mod.rs +++ b/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 NomResult<'a, O> = Result, O>; +type ParseResult<'a, O> = Result, 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>>; +type ConvertResult<'a, O> = std::result::Result>>; 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 { +fn symbol_to_value(s: &Token) -> ConvertResult { 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 { +fn str_to_value(s: &Token) -> ConvertResult { 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, Option, Option), -) -> 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) -> NomResult { +fn symbol_or_expression(input: SliceIter) -> ParseResult { 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) -> NomResult { } } -fn selector_list(input: SliceIter) -> NomResult { +fn selector_list(input: SliceIter) -> ParseResult { 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>, 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>, -) -> 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) -> NomResult { +fn unprefixed_expression(input: SliceIter) -> ParseResult { let _input = input.clone(); either!( input, @@ -831,16 +742,17 @@ fn unprefixed_expression(input: SliceIter) -> NomResult { make_fn!( non_op_expression, Expression>, - alt_peek!( - either!(word!("map"), word!("filter")) => trace_parse!(list_op_expression) | - word!("macro") => trace_parse!(macro_expression) | - word!("module") => trace_parse!(module_expression) | - word!("select") => trace_parse!(select_expression) | - punct!("(") => trace_parse!(grouped_expression) | - trace_parse!(unprefixed_expression)) + either!( + trace_parse!(list_op_expression), + trace_parse!(macro_expression), + trace_parse!(module_expression), + trace_parse!(select_expression), + trace_parse!(grouped_expression), + trace_parse!(unprefixed_expression) + ) ); -fn expression(input: SliceIter) -> NomResult { +fn expression(input: SliceIter) -> ParseResult { 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) -> Result, Statement> { - return alt_peek!(i, - word!("assert") => trace_parse!(assert_statement) | - word!("import") => trace_parse!(import_statement) | - word!("let") => trace_parse!(let_statement) | - word!("out") => trace_parse!(out_statement) | + return either!( + i, + trace_parse!(assert_statement), + trace_parse!(import_statement), + trace_parse!(let_statement), + trace_parse!(out_statement), trace_parse!(expression_statement) ); } diff --git a/src/parse/precedence.rs b/src/parse/precedence.rs index 0d760cf..58eb025 100644 --- a/src/parse/precedence.rs +++ b/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. -fn parse_operand_list<'a>(i: SliceIter<'a, Token>) -> NomResult<'a, Vec> { +fn parse_operand_list<'a>(i: SliceIter<'a, Token>) -> ParseResult<'a, Vec> { // 1. First try to parse a non_op_expression, let mut _i = i.clone(); let mut list = Vec::new();