feat: do compile time math for range neq #469
Conversation
b1ek
requested review from
Mte90,
CymDeveloppement,
Ph0enixKM,
mks-h and
KrosFire
|
Other than these two nitpicks, LGTM. |
|
@Ph0enixKM @KrosFire please re review |
| @@ -59,7 +60,10 @@ impl TranslateModule for Range { | |||
| let from = self.from.translate(meta); | |||
| let to = self.to.translate(meta); | |||
| if self.neq { | |||
| let to_neq = translate_computation(meta, ArithOp::Sub, Some(to), Some("1".to_string())); | |||
| let to_neq = match &self.to.value.as_ref().unwrap() { | |||
| ExprType::Number(_) => (to.parse::<usize>().unwrap() - 1).to_string(), | |||
There was a problem hiding this comment.
This part should be better handled. The ExprType::Number can be for instance:
0.1-123
In each case this unwrap will panic. I think that you should first parse left hand side and right hand side to usize without panic and also compare if a < b, then return the b-1 and otherwise fallback to the translate_computation method.
The problem with floating point numbers will come to end when we introduce the integer types. I'm finishing the divide function for fixed precision real numbers in Bash. I think that we could get rid of bc dependency.
There was a problem hiding this comment.
I think this is out of scope for this PR, as it's part of a wider problem.
There was a problem hiding this comment.
In the meantime, I've changed the unwrap() call to one of my favourite Rust features, unwrap_or_default().
There was a problem hiding this comment.
Seems to work as well as can be expected with negative and float values:
hwalters@Ghostwheel ~/git/amber-lang (range-neq-opt)
$ cat range.ab
#!/usr/bin/env amber
loop num in -5..-1 { echo num }
loop num in 2.3..=5.7 { echo num }
hwalters@Ghostwheel ~/git/amber-lang (range-neq-opt)
$ ./range.ab
-5
-4
-3
-2
2.3
3.3
4.3
5.3
| @@ -59,7 +61,11 @@ impl TranslateModule for Range { | |||
| let from = self.from.translate(meta); | |||
| let to = self.to.translate(meta); | |||
| if self.neq { | |||
| let to_neq = translate_computation(meta, ArithOp::Sub, Some(to), Some("1".to_string())); | |||
| let to_neq = if let Some(ExprType::Number(_)) = &self.to.value { | |||
| to.parse::<isize>().unwrap_or_default().sub(1).to_string() | |||
There was a problem hiding this comment.
I think that it should try to parse and when failed, fallback. Without any unwrapping.
match (&self.to.value, to.parse::<isize>()) {
(Some(ExprType::Number(_)), Some(value) => ...
_ => ...
}There was a problem hiding this comment.
wouldn't this also return 0 without any warning, which will result in undefined behaviour?
There was a problem hiding this comment.
We agreed that the current solution (i.e. your code with isize instead of usize, and some minor refactoring) was good enough for now.
Co-authored-by: Maksym Hazevych <dpadar@protonmail.com>
hdwalters
force-pushed
the
range-neq-opt
branch
from
3afc64e to
9a772dd
Compare
hdwalters
dismissed
KrosFire’s stale review
The only objection has been resolved.
* feat: do compile time math for range neq * Update src/modules/expression/binop/range.rs Co-authored-by: Maksym Hazevych <dpadar@protonmail.com> * allow anything other than Num literal in range neq * use usize instead of i64 * use isize instead of usize --------- Co-authored-by: Maksym Hazevych <dpadar@protonmail.com> Co-authored-by: Huw Walters <huw.walters@gmail.com>
amber:
0..2from:
$(seq 0 $(echo 2 '-' 1 | bc -l | sed '/\./ s/\.\{0,1\}0\{1,\}$//'))to:
$(seq 0 1)(when the
tois a number literal, if its a variable it will generate as before)