Review (and fix some) TODOs in the codebase

modules/units/currencies.nbt

@@ -5,7 +5,8 @@ use units::currency
 # are blocking. In the CLI application, we do however load this module asynchronously after
 # prefetching the exchange rates.
 
-# TODO: if we ever support strings in the language, use 'exchange_rate("USD")', … instead
+# TODO: once we support strings as function parameters, use 'exchange_rate("USD")' and
+# similar instead of separate functions.
 fn exchange_rate_USD() -> Scalar
 fn exchange_rate_JPY() -> Scalar
 fn exchange_rate_GBP() -> Scalar

numbat-cli/src/main.rs

@@ -459,7 +459,7 @@ impl Cli {
 
     fn get_config_path() -> PathBuf {
         let config_dir = dirs::config_dir().unwrap_or_else(|| PathBuf::from("."));
-        config_dir.join("numbat") // TODO: allow for preludes in system paths, user paths, …
+        config_dir.join("numbat")
     }
 
     fn get_modules_paths() -> Vec<PathBuf> {

numbat/src/gamma.rs

@@ -4,7 +4,9 @@ extern "C" {
     fn tgamma(n: c_double) -> c_double;
 }
 
-// TODO: This will be part of the standard in the future: https://github.com/rust-lang/rust/pull/99747
+// TODO: This will be part of the standard in the future [1] [2]
+// [1] https://github.com/rust-lang/rfcs/issues/864
+// [2] https://github.com/rust-lang/rust/pull/99747
 pub fn gamma(x: f64) -> f64 {
     unsafe { tgamma(x) }
 }

numbat/src/interpreter.rs

@@ -177,7 +177,7 @@ mod tests {
 
         assert_evaluates_to(
             "2 meter + 3 meter",
-            (Quantity::from_scalar(2.0 + 3.0) * Quantity::from_unit(Unit::meter())).unwrap(),
+            Quantity::from_scalar(2.0 + 3.0) * Quantity::from_unit(Unit::meter()),
         );
 
         assert_evaluates_to(
@@ -185,17 +185,14 @@ mod tests {
              @aliases(px: short)
              unit pixel : Pixel
              2 * pixel",
-            (Quantity::from_scalar(2.0) * Quantity::from_unit(Unit::new_base("pixel", "px")))
-                .unwrap(),
+            Quantity::from_scalar(2.0) * Quantity::from_unit(Unit::new_base("pixel", "px")),
         );
 
         assert_evaluates_to(
             "fn speed(distance: Length, time: Time) -> Speed = distance / time
              speed(10 * meter, 2 * second)",
-            (Quantity::from_scalar(5.0)
-                * (Quantity::from_unit(Unit::meter()) / Quantity::from_unit(Unit::second()))
-                    .unwrap())
-            .unwrap(),
+            Quantity::from_scalar(5.0)
+                * (Quantity::from_unit(Unit::meter()) / Quantity::from_unit(Unit::second())),
         );
     }
 

numbat/src/parser.rs

@@ -1122,8 +1122,6 @@ impl<'a> Parser<'a> {
     }
 
     fn dimension_exponent(&mut self) -> Result<(Span, Exponent)> {
-        // TODO: potentially allow for ², ³, etc.
-
         if let Some(token) = self.match_exact(TokenKind::Number) {
             let span = self.last().unwrap().span;
             let num_str = token.lexeme.replace('_', "");

numbat/src/prefix_parser.rs

@@ -95,7 +95,7 @@ impl PrefixParser {
                 ("femto", "f", Prefix::Metric(-15)),
                 ("pico", "p", Prefix::Metric(-12)),
                 ("nano", "n", Prefix::Metric(-9)),
-                ("micro", "µ", Prefix::Metric(-6)), // TODO: support 'u' as well. and other unicode characters
+                ("micro", "µ", Prefix::Metric(-6)),
                 ("milli", "m", Prefix::Metric(-3)),
                 ("centi", "c", Prefix::Metric(-2)),
                 ("deci", "d", Prefix::Metric(-1)),

numbat/src/prefix_transformer.rs

@@ -18,7 +18,6 @@ pub(crate) struct Transformer {
     pub dimension_names: Vec<String>,
 }
 
-// TODO: generalize this to a general-purpose transformer (not just for prefixes, could also be used for optimization, inlining, span-replacement, etc.), e.g. using visitor pattern
 impl Transformer {
     pub fn new() -> Self {
         Self {

numbat/src/quantity.rs

@@ -106,7 +106,6 @@ impl Quantity {
 
             let quantity_base_unit_representation = (self.clone()
                 / Quantity::from_unit(common_unit_factors))
-            .unwrap()
             .to_base_unit_representation();
             let own_base_unit_representation = own_unit_reduced.to_base_unit_representation().0;
 
@@ -227,6 +226,14 @@ impl Quantity {
             ),
         ))
     }
+
+    pub fn checked_div(self, other: Self) -> Option<Self> {
+        if other.is_zero() {
+            None
+        } else {
+            Some(self / other)
+        }
+    }
 }
 
 impl From<&Number> for Quantity {
@@ -258,24 +265,24 @@ impl std::ops::Sub for &Quantity {
 }
 
 impl std::ops::Mul for Quantity {
-    type Output = Result<Quantity>;
+    type Output = Quantity;
 
     fn mul(self, rhs: Self) -> Self::Output {
-        Ok(Quantity {
+        Quantity {
             value: self.value * rhs.value,
             unit: self.unit * rhs.unit,
-        })
+        }
     }
 }
 
 impl std::ops::Div for Quantity {
-    type Output = Result<Quantity>;
+    type Output = Quantity;
 
     fn div(self, rhs: Self) -> Self::Output {
-        Ok(Quantity {
+        Quantity {
             value: self.value / rhs.value,
             unit: self.unit / rhs.unit,
-        })
+        }
     }
 }
 

numbat/src/vm.rs

@@ -580,15 +580,10 @@ impl Vm {
                     let result = match op {
                         Op::Add => &lhs + &rhs,
                         Op::Subtract => &lhs - &rhs,
-                        Op::Multiply => lhs * rhs,
-                        Op::Divide => {
-                            // TODO: should this be implemented in Quantity::div?
-                            if rhs.is_zero() {
-                                return Err(RuntimeError::DivisionByZero);
-                            } else {
-                                lhs / rhs
-                            }
-                        }
+                        Op::Multiply => Ok(lhs * rhs),
+                        Op::Divide => Ok(lhs
+                            .checked_div(rhs)
+                            .ok_or_else(|| RuntimeError::DivisionByZero)?),
                         Op::Power => lhs.power(rhs),
                         Op::ConvertTo => lhs.convert_to(rhs.unit()),
                         _ => unreachable!(),