From 117ab7737ac1aa61ef4759837ee7718ba016f860 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sat, 2 Dec 2023 17:25:57 +0000 Subject: [PATCH 01/11] Optimize expr for numerical values --- src/uu/expr/src/expr.rs | 2 +- src/uu/expr/src/syntax_tree.rs | 106 +++++++++++++++++++++------------ 2 files changed, 69 insertions(+), 39 deletions(-) diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index c271f0935..91d7a8788 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -108,7 +108,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .map(|v| v.into_iter().map(|s| s.as_ref()).collect::>()) .unwrap_or_default(); - let res = AstNode::parse(&token_strings)?.eval()?; + let res = AstNode::parse(&token_strings)?.eval()?.to_string(); println!("{res}"); if !is_truthy(&res) { return Err(1.into()); diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index f81f1da1e..705864f3a 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -6,6 +6,7 @@ // spell-checker:ignore (ToDO) ints paren prec multibytes use num_bigint::BigInt; +use num_traits::ToPrimitive; use onig::{Regex, RegexOptions, Syntax}; use crate::{ExprError, ExprResult}; @@ -45,7 +46,7 @@ pub enum StringOp { } impl BinOp { - fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { + fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { match self { Self::Relation(op) => op.eval(left, right), Self::Numeric(op) => op.eval(left, right), @@ -55,10 +56,10 @@ impl BinOp { } impl RelationOp { - fn eval(&self, a: &AstNode, b: &AstNode) -> ExprResult { + fn eval(&self, a: &AstNode, b: &AstNode) -> ExprResult { let a = a.eval()?; let b = b.eval()?; - let b = if let (Ok(a), Ok(b)) = (a.parse::(), b.parse::()) { + let b = if let (NumOrStr::Num(a), NumOrStr::Num(b)) = (&a, &b) { match self { Self::Lt => a < b, Self::Leq => a <= b, @@ -79,24 +80,22 @@ impl RelationOp { } }; if b { - Ok("1".into()) + Ok(NumOrStr::Num(BigInt::from(1))) } else { - Ok("0".into()) + Ok(NumOrStr::Num(BigInt::from(0))) } } } impl NumericOp { - fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { + fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { let a: BigInt = left .eval()? - .parse() - .map_err(|_| ExprError::NonIntegerArgument)?; + .to_bigint()?; let b: BigInt = right .eval()? - .parse() - .map_err(|_| ExprError::NonIntegerArgument)?; - Ok(match self { + .to_bigint()?; + Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, Self::Mul => a * b, @@ -110,67 +109,66 @@ impl NumericOp { }; a % b } - } - .to_string()) + })) } } impl StringOp { - fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { + fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { match self { Self::Or => { let left = left.eval()?; - if is_truthy(&left) { + if is_truthy(&left.to_string()) { return Ok(left); } let right = right.eval()?; - if is_truthy(&right) { + if is_truthy(&right.to_string()) { return Ok(right); } - Ok("0".into()) + Ok(NumOrStr::Num(BigInt::from(0))) } Self::And => { let left = left.eval()?; - if !is_truthy(&left) { - return Ok("0".into()); + if !is_truthy(&left.to_string()) { + return Ok(NumOrStr::Num(BigInt::from(0))); } let right = right.eval()?; - if !is_truthy(&right) { - return Ok("0".into()); + if !is_truthy(&right.to_string()) { + return Ok(NumOrStr::Num(BigInt::from(0))); } Ok(left) } Self::Match => { let left = left.eval()?; let right = right.eval()?; - let re_string = format!("^{}", &right); + let re_string = format!("^{}", right.to_string()); let re = Regex::with_options( &re_string, RegexOptions::REGEX_OPTION_NONE, Syntax::grep(), ) .map_err(|_| ExprError::InvalidRegexExpression)?; - Ok(if re.captures_len() > 0 { - re.captures(&left) + Ok(NumOrStr::Str(if re.captures_len() > 0 { + re.captures(&left.to_string()) .map(|captures| captures.at(1).unwrap()) .unwrap_or("") .to_string() } else { - re.find(&left) + re.find(&left.to_string()) .map_or("0".to_string(), |(start, end)| (end - start).to_string()) - }) + })) } Self::Index => { let left = left.eval()?; let right = right.eval()?; - for (current_idx, ch_h) in left.chars().enumerate() { - for ch_n in right.chars() { + for (current_idx, ch_h) in left.to_string().chars().enumerate() { + for ch_n in right.to_string().chars() { if ch_n == ch_h { - return Ok((current_idx + 1).to_string()); + return Ok(NumOrStr::Num(BigInt::from(current_idx + 1))); } } } - Ok("0".to_string()) + Ok(NumOrStr::Num(BigInt::from(0))) } } } @@ -200,6 +198,38 @@ const PRECEDENCE: &[&[(&str, BinOp)]] = &[ &[(":", BinOp::String(StringOp::Match))], ]; +#[derive(Debug, PartialEq, Eq, Ord, PartialOrd)] +pub enum NumOrStr { + Num(BigInt), + Str(String), +} + +impl NumOrStr { + pub fn to_usize(self: NumOrStr) -> Option { + match self.to_bigint() { + Ok(num) => {num.to_usize()} + Err(_) => {None}, + } + } + + pub fn to_string(self: &NumOrStr) -> String { + match self { + NumOrStr::Num(num) => {num.to_string()} + NumOrStr::Str(str) => {str.to_string()}, + } + } + + pub fn to_bigint(self: NumOrStr) -> ExprResult { + match self { + NumOrStr::Num(num) => {Ok(num)} + NumOrStr::Str(str) => { match str.parse::() { + Ok(val) => {Ok(val)}, + Err(_) => {Err(ExprError::NonIntegerArgument)} + }}, + } + } +} + #[derive(Debug, PartialEq, Eq)] pub enum AstNode { Leaf { @@ -225,9 +255,9 @@ impl AstNode { Parser::new(input).parse() } - pub fn eval(&self) -> ExprResult { + pub fn eval(&self) -> ExprResult { match self { - Self::Leaf { value } => Ok(value.into()), + Self::Leaf { value } => Ok(NumOrStr::Str(value.to_string())), Self::BinOp { op_type, left, @@ -238,7 +268,7 @@ impl AstNode { pos, length, } => { - let string = string.eval()?; + let string = string.eval()?.to_string(); // The GNU docs say: // @@ -247,16 +277,16 @@ impl AstNode { // // So we coerce errors into 0 to make that the only case we // have to care about. - let pos: usize = pos.eval()?.parse().unwrap_or(0); - let length: usize = length.eval()?.parse().unwrap_or(0); + let pos: usize = pos.eval()?.to_usize().unwrap_or(0); + let length: usize = length.eval()?.to_usize().unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { - return Ok(String::new()); + return Ok(NumOrStr::Str(String::new())); }; - Ok(string.chars().skip(pos).take(length).collect()) + Ok(NumOrStr::Str(string.chars().skip(pos).take(length).collect())) } - Self::Length { string } => Ok(string.eval()?.chars().count().to_string()), + Self::Length { string } => Ok(NumOrStr::Num(BigInt::from(string.eval()?.to_string().chars().count()))), } } } From d8a64a90ece80fc029860f66d7bf8858c79f9e91 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sun, 3 Dec 2023 15:09:12 +0000 Subject: [PATCH 02/11] Formatting fixes in expr --- src/uu/expr/src/syntax_tree.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 705864f3a..1c74b9710 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -89,12 +89,8 @@ impl RelationOp { impl NumericOp { fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { - let a: BigInt = left - .eval()? - .to_bigint()?; - let b: BigInt = right - .eval()? - .to_bigint()?; + let a: BigInt = left.eval()?.to_bigint()?; + let b: BigInt = right.eval()?.to_bigint()?; Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, @@ -207,25 +203,25 @@ pub enum NumOrStr { impl NumOrStr { pub fn to_usize(self: NumOrStr) -> Option { match self.to_bigint() { - Ok(num) => {num.to_usize()} - Err(_) => {None}, + Ok(num) => num.to_usize(), + Err(_) => None, } } pub fn to_string(self: &NumOrStr) -> String { match self { - NumOrStr::Num(num) => {num.to_string()} - NumOrStr::Str(str) => {str.to_string()}, + NumOrStr::Num(num) => num.to_string(), + NumOrStr::Str(str) => str.to_string(), } } pub fn to_bigint(self: NumOrStr) -> ExprResult { match self { - NumOrStr::Num(num) => {Ok(num)} - NumOrStr::Str(str) => { match str.parse::() { - Ok(val) => {Ok(val)}, - Err(_) => {Err(ExprError::NonIntegerArgument)} - }}, + NumOrStr::Num(num) => Ok(num), + NumOrStr::Str(str) => match str.parse::() { + Ok(val) => Ok(val), + Err(_) => Err(ExprError::NonIntegerArgument), + }, } } } @@ -284,9 +280,13 @@ impl AstNode { return Ok(NumOrStr::Str(String::new())); }; - Ok(NumOrStr::Str(string.chars().skip(pos).take(length).collect())) + Ok(NumOrStr::Str( + string.chars().skip(pos).take(length).collect(), + )) } - Self::Length { string } => Ok(NumOrStr::Num(BigInt::from(string.eval()?.to_string().chars().count()))), + Self::Length { string } => Ok(NumOrStr::Num(BigInt::from( + string.eval()?.to_string().chars().count(), + ))), } } } From f8573d555133f22cfdfbfa198a265138277d3f36 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sun, 3 Dec 2023 20:03:50 +0000 Subject: [PATCH 03/11] code and styling fixes in expr --- src/uu/expr/src/expr.rs | 4 +- src/uu/expr/src/syntax_tree.rs | 130 ++++++++++++++++++++------------- 2 files changed, 83 insertions(+), 51 deletions(-) diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index 91d7a8788..b46034f84 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -13,7 +13,7 @@ use uucore::{ format_usage, help_about, help_section, help_usage, }; -use crate::syntax_tree::is_truthy; +use crate::syntax_tree::{is_truthy, NumOrStr}; mod syntax_tree; @@ -110,7 +110,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let res = AstNode::parse(&token_strings)?.eval()?.to_string(); println!("{res}"); - if !is_truthy(&res) { + if !is_truthy(&NumOrStr::from(res)) { return Err(1.into()); } Ok(()) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 1c74b9710..79ba8d9ae 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -57,8 +57,8 @@ impl BinOp { impl RelationOp { fn eval(&self, a: &AstNode, b: &AstNode) -> ExprResult { - let a = a.eval()?; - let b = b.eval()?; + let a = a.eval()?.coerce_num(); + let b = b.eval()?.coerce_num(); let b = if let (NumOrStr::Num(a), NumOrStr::Num(b)) = (&a, &b) { match self { Self::Lt => a < b, @@ -80,17 +80,17 @@ impl RelationOp { } }; if b { - Ok(NumOrStr::Num(BigInt::from(1))) + Ok(NumOrStr::from(1)) } else { - Ok(NumOrStr::Num(BigInt::from(0))) + Ok(NumOrStr::from(0)) } } } impl NumericOp { fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { - let a: BigInt = left.eval()?.to_bigint()?; - let b: BigInt = right.eval()?.to_bigint()?; + let a = left.eval()?.to_bigint()?; + let b = right.eval()?.to_bigint()?; Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, @@ -114,23 +114,23 @@ impl StringOp { match self { Self::Or => { let left = left.eval()?; - if is_truthy(&left.to_string()) { + if is_truthy(&left) { return Ok(left); } let right = right.eval()?; - if is_truthy(&right.to_string()) { + if is_truthy(&right) { return Ok(right); } - Ok(NumOrStr::Num(BigInt::from(0))) + Ok(NumOrStr::from(0)) } Self::And => { let left = left.eval()?; - if !is_truthy(&left.to_string()) { - return Ok(NumOrStr::Num(BigInt::from(0))); + if !is_truthy(&left) { + return Ok(NumOrStr::from(0)); } let right = right.eval()?; - if !is_truthy(&right.to_string()) { - return Ok(NumOrStr::Num(BigInt::from(0))); + if !is_truthy(&right) { + return Ok(NumOrStr::from(0)); } Ok(left) } @@ -144,7 +144,7 @@ impl StringOp { Syntax::grep(), ) .map_err(|_| ExprError::InvalidRegexExpression)?; - Ok(NumOrStr::Str(if re.captures_len() > 0 { + Ok(NumOrStr::from(if re.captures_len() > 0 { re.captures(&left.to_string()) .map(|captures| captures.at(1).unwrap()) .unwrap_or("") @@ -155,16 +155,16 @@ impl StringOp { })) } Self::Index => { - let left = left.eval()?; - let right = right.eval()?; - for (current_idx, ch_h) in left.to_string().chars().enumerate() { + let left = left.eval()?.to_string(); + let right = right.eval()?.to_string(); + for (current_idx, ch_h) in left.chars().enumerate() { for ch_n in right.to_string().chars() { if ch_n == ch_h { - return Ok(NumOrStr::Num(BigInt::from(current_idx + 1))); + return Ok(NumOrStr::from(current_idx + 1)); } } } - Ok(NumOrStr::Num(BigInt::from(0))) + Ok(NumOrStr::from(0)) } } } @@ -200,27 +200,54 @@ pub enum NumOrStr { Str(String), } +impl From for NumOrStr { + fn from(num: usize) -> NumOrStr { + NumOrStr::Num(BigInt::from(num)) + } +} + +impl From for NumOrStr { + fn from(num: BigInt) -> NumOrStr { + NumOrStr::Num(num) + } +} + +impl From for NumOrStr { + fn from(str: String) -> NumOrStr { + NumOrStr::Str(str) + } +} + impl NumOrStr { - pub fn to_usize(self: NumOrStr) -> Option { + pub fn to_usize(self: Self) -> Option { match self.to_bigint() { Ok(num) => num.to_usize(), Err(_) => None, } } - pub fn to_string(self: &NumOrStr) -> String { + pub fn to_string(self: Self) -> String { match self { - NumOrStr::Num(num) => num.to_string(), - NumOrStr::Str(str) => str.to_string(), + Self::Num(num) => num.to_string(), + Self::Str(str) => str.to_string(), } } - pub fn to_bigint(self: NumOrStr) -> ExprResult { + pub fn to_bigint(self: Self) -> ExprResult { match self { - NumOrStr::Num(num) => Ok(num), - NumOrStr::Str(str) => match str.parse::() { - Ok(val) => Ok(val), - Err(_) => Err(ExprError::NonIntegerArgument), + Self::Num(num) => Ok(num), + Self::Str(str) => str + .parse::() + .map_err(|_| ExprError::NonIntegerArgument), + } + } + + pub fn coerce_num(self: Self) -> NumOrStr { + match self { + Self::Num(num) => Self::from(num), + Self::Str(str) => match str.parse::() { + Ok(num) => Self::from(num), + Err(_) => Self::from(str), }, } } @@ -253,7 +280,7 @@ impl AstNode { pub fn eval(&self) -> ExprResult { match self { - Self::Leaf { value } => Ok(NumOrStr::Str(value.to_string())), + Self::Leaf { value } => Ok(NumOrStr::from(value.to_string())), Self::BinOp { op_type, left, @@ -277,16 +304,16 @@ impl AstNode { let length: usize = length.eval()?.to_usize().unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { - return Ok(NumOrStr::Str(String::new())); + return Ok(NumOrStr::from(String::new())); }; - Ok(NumOrStr::Str( - string.chars().skip(pos).take(length).collect(), + Ok(NumOrStr::from( + string.chars().skip(pos).take(length).collect::(), )) } - Self::Length { string } => Ok(NumOrStr::Num(BigInt::from( - string.eval()?.to_string().chars().count(), - ))), + Self::Length { string } => { + Ok(NumOrStr::from(string.eval()?.to_string().chars().count())) + } } } } @@ -429,21 +456,26 @@ impl<'a> Parser<'a> { /// Determine whether `expr` should evaluate the string as "truthy" /// /// Truthy strings are either empty or match the regex "-?0+". -pub fn is_truthy(s: &str) -> bool { - // Edge case: `-` followed by nothing is truthy - if s == "-" { - return true; +pub fn is_truthy(s: &NumOrStr) -> bool { + match s { + NumOrStr::Num(num) => num == &BigInt::from(0), + NumOrStr::Str(str) => { + // Edge case: `-` followed by nothing is truthy + if str == "-" { + return true; + } + + let mut bytes = str.bytes(); + + // Empty string is falsy + let Some(first) = bytes.next() else { + return false; + }; + + let is_zero = (first == b'-' || first == b'0') && bytes.all(|b| b == b'0'); + !is_zero + } } - - let mut bytes = s.bytes(); - - // Empty string is falsy - let Some(first) = bytes.next() else { - return false; - }; - - let is_zero = (first == b'-' || first == b'0') && bytes.all(|b| b == b'0'); - !is_zero } #[cfg(test)] From 5672e3d9bdec3acc1d1ab22b0217b5fac17ab10b Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sun, 3 Dec 2023 22:07:56 +0000 Subject: [PATCH 04/11] Fix errors --- src/uu/expr/src/syntax_tree.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 79ba8d9ae..a4cb99a83 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -5,7 +5,7 @@ // spell-checker:ignore (ToDO) ints paren prec multibytes -use num_bigint::BigInt; +use num_bigint::{BigInt, ParseBigIntError}; use num_traits::ToPrimitive; use onig::{Regex, RegexOptions, Syntax}; @@ -57,9 +57,9 @@ impl BinOp { impl RelationOp { fn eval(&self, a: &AstNode, b: &AstNode) -> ExprResult { - let a = a.eval()?.coerce_num(); - let b = b.eval()?.coerce_num(); - let b = if let (NumOrStr::Num(a), NumOrStr::Num(b)) = (&a, &b) { + let a = a.eval()?; + let b = b.eval()?; + let b = if let (Ok(a), Ok(b)) = (&a.coerce_bigint(), &b.coerce_bigint()) { match self { Self::Lt => a < b, Self::Leq => a <= b, @@ -242,13 +242,10 @@ impl NumOrStr { } } - pub fn coerce_num(self: Self) -> NumOrStr { + pub fn coerce_bigint(self: &Self) -> Result { match self { - Self::Num(num) => Self::from(num), - Self::Str(str) => match str.parse::() { - Ok(num) => Self::from(num), - Err(_) => Self::from(str), - }, + Self::Num(num) => Ok(num.clone()), + Self::Str(str) => str.parse::(), } } } @@ -458,7 +455,7 @@ impl<'a> Parser<'a> { /// Truthy strings are either empty or match the regex "-?0+". pub fn is_truthy(s: &NumOrStr) -> bool { match s { - NumOrStr::Num(num) => num == &BigInt::from(0), + NumOrStr::Num(num) => num != &BigInt::from(0), NumOrStr::Str(str) => { // Edge case: `-` followed by nothing is truthy if str == "-" { From 21c041fa79b64d9f55b8672c4a74e22dd941fd96 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sun, 3 Dec 2023 22:27:13 +0000 Subject: [PATCH 05/11] Fix lint issues in expr --- src/uu/expr/src/syntax_tree.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index a4cb99a83..4f447e60f 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -201,39 +201,39 @@ pub enum NumOrStr { } impl From for NumOrStr { - fn from(num: usize) -> NumOrStr { - NumOrStr::Num(BigInt::from(num)) + fn from(num: usize) -> Self { + Self::Num(BigInt::from(num)) } } impl From for NumOrStr { - fn from(num: BigInt) -> NumOrStr { - NumOrStr::Num(num) + fn from(num: BigInt) -> Self { + Self::Num(num) } } impl From for NumOrStr { - fn from(str: String) -> NumOrStr { - NumOrStr::Str(str) + fn from(str: String) -> Self { + Self::Str(str) } } impl NumOrStr { - pub fn to_usize(self: Self) -> Option { + pub fn to_usize(self) -> Option { match self.to_bigint() { Ok(num) => num.to_usize(), Err(_) => None, } } - pub fn to_string(self: Self) -> String { + pub fn to_string(self) -> String { match self { Self::Num(num) => num.to_string(), Self::Str(str) => str.to_string(), } } - pub fn to_bigint(self: Self) -> ExprResult { + pub fn to_bigint(self) -> ExprResult { match self { Self::Num(num) => Ok(num), Self::Str(str) => str @@ -242,7 +242,7 @@ impl NumOrStr { } } - pub fn coerce_bigint(self: &Self) -> Result { + pub fn coerce_bigint(&self) -> Result { match self { Self::Num(num) => Ok(num.clone()), Self::Str(str) => str.parse::(), From 9ecd6a296e06b6f20a5bc29f876a546cdafd020d Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Sun, 3 Dec 2023 23:32:51 +0000 Subject: [PATCH 06/11] Refactoring for lint issues --- src/uu/expr/src/expr.rs | 2 +- src/uu/expr/src/syntax_tree.rs | 53 ++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index b46034f84..1a9bb07de 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -108,7 +108,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .map(|v| v.into_iter().map(|s| s.as_ref()).collect::>()) .unwrap_or_default(); - let res = AstNode::parse(&token_strings)?.eval()?.to_string(); + let res: String = AstNode::parse(&token_strings)?.eval()?.into(); println!("{res}"); if !is_truthy(&NumOrStr::from(res)) { return Err(1.into()); diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 4f447e60f..7677b5e7e 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -59,7 +59,7 @@ impl RelationOp { fn eval(&self, a: &AstNode, b: &AstNode) -> ExprResult { let a = a.eval()?; let b = b.eval()?; - let b = if let (Ok(a), Ok(b)) = (&a.coerce_bigint(), &b.coerce_bigint()) { + let b = if let (Ok(a), Ok(b)) = (&a.to_bigint(), &b.to_bigint()) { match self { Self::Lt => a < b, Self::Leq => a <= b, @@ -89,8 +89,8 @@ impl RelationOp { impl NumericOp { fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { - let a = left.eval()?.to_bigint()?; - let b = right.eval()?.to_bigint()?; + let a = >>::into(left.eval()?)?; + let b = >>::into(right.eval()?)?; Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, @@ -135,9 +135,9 @@ impl StringOp { Ok(left) } Self::Match => { - let left = left.eval()?; - let right = right.eval()?; - let re_string = format!("^{}", right.to_string()); + let left: String = left.eval()?.into(); + let right: String = right.eval()?.into(); + let re_string = format!("^{}", right); let re = Regex::with_options( &re_string, RegexOptions::REGEX_OPTION_NONE, @@ -145,18 +145,18 @@ impl StringOp { ) .map_err(|_| ExprError::InvalidRegexExpression)?; Ok(NumOrStr::from(if re.captures_len() > 0 { - re.captures(&left.to_string()) + re.captures(&left) .map(|captures| captures.at(1).unwrap()) .unwrap_or("") .to_string() } else { - re.find(&left.to_string()) + re.find(&left) .map_or("0".to_string(), |(start, end)| (end - start).to_string()) })) } Self::Index => { - let left = left.eval()?.to_string(); - let right = right.eval()?.to_string(); + let left: String = left.eval()?.into(); + let right: String = right.eval()?.into(); for (current_idx, ch_h) in left.chars().enumerate() { for ch_n in right.to_string().chars() { if ch_n == ch_h { @@ -218,22 +218,26 @@ impl From for NumOrStr { } } -impl NumOrStr { - pub fn to_usize(self) -> Option { - match self.to_bigint() { +impl Into> for NumOrStr { + fn into(self) -> Option { + match self.into() { Ok(num) => num.to_usize(), Err(_) => None, } } +} - pub fn to_string(self) -> String { +impl Into for NumOrStr { + fn into(self) -> String { match self { Self::Num(num) => num.to_string(), Self::Str(str) => str.to_string(), } } +} - pub fn to_bigint(self) -> ExprResult { +impl Into> for NumOrStr { + fn into(self) -> ExprResult { match self { Self::Num(num) => Ok(num), Self::Str(str) => str @@ -241,8 +245,10 @@ impl NumOrStr { .map_err(|_| ExprError::NonIntegerArgument), } } +} - pub fn coerce_bigint(&self) -> Result { +impl NumOrStr { + pub fn to_bigint(&self) -> Result { match self { Self::Num(num) => Ok(num.clone()), Self::Str(str) => str.parse::(), @@ -288,7 +294,7 @@ impl AstNode { pos, length, } => { - let string = string.eval()?.to_string(); + let string: String = string.eval()?.into(); // The GNU docs say: // @@ -297,8 +303,9 @@ impl AstNode { // // So we coerce errors into 0 to make that the only case we // have to care about. - let pos: usize = pos.eval()?.to_usize().unwrap_or(0); - let length: usize = length.eval()?.to_usize().unwrap_or(0); + let pos: usize = >>::into(pos.eval()?).unwrap_or(0); + let length: usize = + >>::into(length.eval()?).unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { return Ok(NumOrStr::from(String::new())); @@ -308,9 +315,11 @@ impl AstNode { string.chars().skip(pos).take(length).collect::(), )) } - Self::Length { string } => { - Ok(NumOrStr::from(string.eval()?.to_string().chars().count())) - } + Self::Length { string } => Ok(NumOrStr::from( + >::into(string.eval()?) + .chars() + .count(), + )), } } } From 4d2ae8485cd65429e64606119acac3156158ea2b Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Mon, 4 Dec 2023 22:44:18 +0000 Subject: [PATCH 07/11] impl from trait instead of into --- src/uu/expr/src/syntax_tree.rs | 43 ++++++++++++++++------------------ 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 7677b5e7e..ae2a44e52 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -89,8 +89,8 @@ impl RelationOp { impl NumericOp { fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { - let a = >>::into(left.eval()?)?; - let b = >>::into(right.eval()?)?; + let a = ExprResult::::from(left.eval()?)?; + let b = ExprResult::::from(right.eval()?)?; Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, @@ -218,29 +218,29 @@ impl From for NumOrStr { } } -impl Into> for NumOrStr { - fn into(self) -> Option { - match self.into() { +impl From for Option { + fn from(s: NumOrStr) -> Self { + match s.into() { Ok(num) => num.to_usize(), Err(_) => None, } } } -impl Into for NumOrStr { - fn into(self) -> String { - match self { - Self::Num(num) => num.to_string(), - Self::Str(str) => str.to_string(), +impl From for String { + fn from(s: NumOrStr) -> Self { + match s { + NumOrStr::Num(num) => num.to_string(), + NumOrStr::Str(str) => str.to_string(), } } } -impl Into> for NumOrStr { - fn into(self) -> ExprResult { - match self { - Self::Num(num) => Ok(num), - Self::Str(str) => str +impl From for ExprResult { + fn from(s: NumOrStr) -> Self { + match s { + NumOrStr::Num(num) => Ok(num), + NumOrStr::Str(str) => str .parse::() .map_err(|_| ExprError::NonIntegerArgument), } @@ -303,9 +303,8 @@ impl AstNode { // // So we coerce errors into 0 to make that the only case we // have to care about. - let pos: usize = >>::into(pos.eval()?).unwrap_or(0); - let length: usize = - >>::into(length.eval()?).unwrap_or(0); + let pos: usize = Option::::from(pos.eval()?).unwrap_or(0); + let length: usize = Option::::from(length.eval()?).unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { return Ok(NumOrStr::from(String::new())); @@ -315,11 +314,9 @@ impl AstNode { string.chars().skip(pos).take(length).collect::(), )) } - Self::Length { string } => Ok(NumOrStr::from( - >::into(string.eval()?) - .chars() - .count(), - )), + Self::Length { string } => { + Ok(NumOrStr::from(String::from(string.eval()?).chars().count())) + } } } } From fa0c64ddde2d08103c8e682815ec9f5924a716a5 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Mon, 11 Dec 2023 02:05:55 +0000 Subject: [PATCH 08/11] review fixes --- src/uu/expr/src/expr.rs | 6 +-- src/uu/expr/src/syntax_tree.rs | 88 +++++++++++++++++----------------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index 1a9bb07de..6adedd9ec 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -13,7 +13,7 @@ use uucore::{ format_usage, help_about, help_section, help_usage, }; -use crate::syntax_tree::{is_truthy, NumOrStr}; +use crate::syntax_tree::is_truthy; mod syntax_tree; @@ -108,9 +108,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .map(|v| v.into_iter().map(|s| s.as_ref()).collect::>()) .unwrap_or_default(); - let res: String = AstNode::parse(&token_strings)?.eval()?.into(); + let res: String = AstNode::parse(&token_strings)?.eval()?.eval_as_string(); println!("{res}"); - if !is_truthy(&NumOrStr::from(res)) { + if !is_truthy(&res.into()) { return Err(1.into()); } Ok(()) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index ae2a44e52..4514b2a67 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -80,17 +80,17 @@ impl RelationOp { } }; if b { - Ok(NumOrStr::from(1)) + Ok(1.into()) } else { - Ok(NumOrStr::from(0)) + Ok(0.into()) } } } impl NumericOp { fn eval(&self, left: &AstNode, right: &AstNode) -> ExprResult { - let a = ExprResult::::from(left.eval()?)?; - let b = ExprResult::::from(right.eval()?)?; + let a = left.eval()?.eval_as_bigint()?; + let b = right.eval()?.eval_as_bigint()?; Ok(NumOrStr::Num(match self { Self::Add => a + b, Self::Sub => a - b, @@ -121,22 +121,22 @@ impl StringOp { if is_truthy(&right) { return Ok(right); } - Ok(NumOrStr::from(0)) + Ok(0.into()) } Self::And => { let left = left.eval()?; if !is_truthy(&left) { - return Ok(NumOrStr::from(0)); + return Ok(0.into()); } let right = right.eval()?; if !is_truthy(&right) { - return Ok(NumOrStr::from(0)); + return Ok(0.into()); } Ok(left) } Self::Match => { - let left: String = left.eval()?.into(); - let right: String = right.eval()?.into(); + let left = left.eval()?.eval_as_string(); + let right = right.eval()?.eval_as_string(); let re_string = format!("^{}", right); let re = Regex::with_options( &re_string, @@ -144,7 +144,7 @@ impl StringOp { Syntax::grep(), ) .map_err(|_| ExprError::InvalidRegexExpression)?; - Ok(NumOrStr::from(if re.captures_len() > 0 { + Ok(if re.captures_len() > 0 { re.captures(&left) .map(|captures| captures.at(1).unwrap()) .unwrap_or("") @@ -152,19 +152,20 @@ impl StringOp { } else { re.find(&left) .map_or("0".to_string(), |(start, end)| (end - start).to_string()) - })) + } + .into()) } Self::Index => { - let left: String = left.eval()?.into(); - let right: String = right.eval()?.into(); + let left = left.eval()?.eval_as_string(); + let right = right.eval()?.eval_as_string(); for (current_idx, ch_h) in left.chars().enumerate() { for ch_n in right.to_string().chars() { if ch_n == ch_h { - return Ok(NumOrStr::from(current_idx + 1)); + return Ok((current_idx + 1).into()); } } } - Ok(NumOrStr::from(0)) + Ok(0.into()) } } } @@ -220,33 +221,13 @@ impl From for NumOrStr { impl From for Option { fn from(s: NumOrStr) -> Self { - match s.into() { + match s.eval_as_bigint() { Ok(num) => num.to_usize(), Err(_) => None, } } } -impl From for String { - fn from(s: NumOrStr) -> Self { - match s { - NumOrStr::Num(num) => num.to_string(), - NumOrStr::Str(str) => str.to_string(), - } - } -} - -impl From for ExprResult { - fn from(s: NumOrStr) -> Self { - match s { - NumOrStr::Num(num) => Ok(num), - NumOrStr::Str(str) => str - .parse::() - .map_err(|_| ExprError::NonIntegerArgument), - } - } -} - impl NumOrStr { pub fn to_bigint(&self) -> Result { match self { @@ -254,6 +235,22 @@ impl NumOrStr { Self::Str(str) => str.parse::(), } } + + pub fn eval_as_bigint(self) -> ExprResult { + match self { + NumOrStr::Num(num) => Ok(num), + NumOrStr::Str(str) => str + .parse::() + .map_err(|_| ExprError::NonIntegerArgument), + } + } + + pub fn eval_as_string(self) -> String { + match self { + NumOrStr::Num(num) => num.to_string(), + NumOrStr::Str(str) => str, + } + } } #[derive(Debug, PartialEq, Eq)] @@ -283,7 +280,7 @@ impl AstNode { pub fn eval(&self) -> ExprResult { match self { - Self::Leaf { value } => Ok(NumOrStr::from(value.to_string())), + Self::Leaf { value } => Ok(value.to_string().into()), Self::BinOp { op_type, left, @@ -294,7 +291,7 @@ impl AstNode { pos, length, } => { - let string: String = string.eval()?.into(); + let string: String = string.eval()?.eval_as_string(); // The GNU docs say: // @@ -307,16 +304,17 @@ impl AstNode { let length: usize = Option::::from(length.eval()?).unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { - return Ok(NumOrStr::from(String::new())); + return Ok(String::new().into()); }; - Ok(NumOrStr::from( - string.chars().skip(pos).take(length).collect::(), - )) - } - Self::Length { string } => { - Ok(NumOrStr::from(String::from(string.eval()?).chars().count())) + Ok(string + .chars() + .skip(pos) + .take(length) + .collect::() + .into()) } + Self::Length { string } => Ok(string.eval()?.eval_as_string().chars().count().into()), } } } From 824371d8841f1652de5a8b4d865a175af2a91d5b Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Mon, 11 Dec 2023 02:12:24 +0000 Subject: [PATCH 09/11] style lint fixes --- src/uu/expr/src/syntax_tree.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 4514b2a67..820911cd3 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -238,8 +238,8 @@ impl NumOrStr { pub fn eval_as_bigint(self) -> ExprResult { match self { - NumOrStr::Num(num) => Ok(num), - NumOrStr::Str(str) => str + Self::Num(num) => Ok(num), + Self::Str(str) => str .parse::() .map_err(|_| ExprError::NonIntegerArgument), } @@ -247,8 +247,8 @@ impl NumOrStr { pub fn eval_as_string(self) -> String { match self { - NumOrStr::Num(num) => num.to_string(), - NumOrStr::Str(str) => str, + Self::Num(num) => num.to_string(), + Self::Str(str) => str, } } } From 3bf966df56a435a99e05e8802a85be16f42bf100 Mon Sep 17 00:00:00 2001 From: Arpit Bhadauria Date: Mon, 11 Dec 2023 20:47:36 +0000 Subject: [PATCH 10/11] remove from trait for NumOrStr --- src/uu/expr/src/syntax_tree.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 820911cd3..7817b1721 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -219,15 +219,6 @@ impl From for NumOrStr { } } -impl From for Option { - fn from(s: NumOrStr) -> Self { - match s.eval_as_bigint() { - Ok(num) => num.to_usize(), - Err(_) => None, - } - } -} - impl NumOrStr { pub fn to_bigint(&self) -> Result { match self { @@ -300,8 +291,16 @@ impl AstNode { // // So we coerce errors into 0 to make that the only case we // have to care about. - let pos: usize = Option::::from(pos.eval()?).unwrap_or(0); - let length: usize = Option::::from(length.eval()?).unwrap_or(0); + let pos = pos + .eval()? + .eval_as_bigint() + .map_or(0.into(), |n| n.to_usize()) + .unwrap_or(0); + let length = length + .eval()? + .eval_as_bigint() + .map_or(0.into(), |n| n.to_usize()) + .unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else { return Ok(String::new().into()); From 7f23faf8999069b283a370991f1f3220619304ba Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Thu, 14 Dec 2023 16:35:56 +0100 Subject: [PATCH 11/11] expr: clean up conversion from bigint to usize --- src/uu/expr/src/syntax_tree.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/uu/expr/src/syntax_tree.rs b/src/uu/expr/src/syntax_tree.rs index 7817b1721..28e4ff0bd 100644 --- a/src/uu/expr/src/syntax_tree.rs +++ b/src/uu/expr/src/syntax_tree.rs @@ -294,12 +294,14 @@ impl AstNode { let pos = pos .eval()? .eval_as_bigint() - .map_or(0.into(), |n| n.to_usize()) + .ok() + .and_then(|n| n.to_usize()) .unwrap_or(0); let length = length .eval()? .eval_as_bigint() - .map_or(0.into(), |n| n.to_usize()) + .ok() + .and_then(|n| n.to_usize()) .unwrap_or(0); let (Some(pos), Some(_)) = (pos.checked_sub(1), length.checked_sub(1)) else {