landley / toybox

toybox
http://landley.net/toybox
BSD Zero Clause License
2.39k stars 334 forks source link

Expr arithmetic and comparisons working differently, inconsistent errors #454

Open yarcod opened 1 year ago

yarcod commented 1 year ago

Doing some small testing on an Android phone, I noticed that arithmetic operations seem to work as expected, i.e. they use 64 bits, but comparisons seems to only use 32 bits. This means that while a number larger than 2^31 plus another number would work, but comparing those two will give an different result.

To give some examples:

# expr 9223372036854775807 + 1
-9223372036854775808
# expr 11170193408 \< 0
1
# expr 11170193408 + 1
11170193409
# expr 11170193408 - 3221225472
7948967936
# expr 11170193408 \> 322122547
0
1| # expr 11170193408 \> 32212254
0
1| # expr 100000000000 \> 1                                                                                                                                                                 
1

That last example did not throw any error, and that is erroneous from what I can tell.

I am not able to determine exactly when expr returns an error, because it does not always seem to be due to overflows. So, I have two issues to report, perhaps. Both the inconsistent use of 32 and 64 bit integers in comparisons and arithmetic, as well as inconsistent errors being thrown when comparing large numbers.

Or am I just missing something? :)

enh-google commented 1 year ago

no, it's not you... this fixes it:

diff --git a/toys/pending/expr.c b/toys/pending/expr.c
index a236aaab..54ebd14b 100644
--- a/toys/pending/expr.c
+++ b/toys/pending/expr.c
@@ -146,9 +146,8 @@ static struct op_def {

 void eval_op(struct op_def *o, struct value *ret, struct value *rhs)
 {
-  long long a, b, x = 0; // x = a OP b for ints.
+  long long cmp, a, b, x = 0; // x = a OP b for ints.
   char *s, *t; // string operands
-  int cmp;

   switch (o->sig) {

but i think rob's rewriting expr right now anyway?

yarcod commented 1 year ago

Great, thanks! Does this also solve the issue with the inconsistent return codes? Because I am still not sure what they were caused by.

landley commented 1 year ago

Commit 251be88968c1 with a test.

As for rewrite, I have a tree with a dirty expr.c in it but it doesn't collapse cleanly into the $(( )) math logic and I find a lot of its string vs not string behavior non-obvious, so I keep waffling over throw it out vs patch what's there vs try not to touch it...