expand: Support integer bases in arithmetic

[stephen-fox]

The existing arithmetic logic is hard coded to always treat strings as base 10 integers. This commit updates that logic to support what bash supports based on its manual page. [1]

References

1. https://www.man7.org/linux/man-pages/man1/bash.1.html

[mvdan]

What shell syntax does this follow? I don't see any mention of 0b in man bash for example.

[stephen-fox]

Hello @mdvan. The 0b is something I picked up from Go. If we want to adhere strictly to bash, this is what the manual page says:

Integer constants follow the C language definition, without
suffixes or character constants. Constants with a leading 0 are
interpreted as octal numbers. A leading 0x or 0X denotes
hexadecimal. Otherwise, numbers take the form [base#]n, where the
optional base is a decimal number between 2 and 64 representing
the arithmetic base, and n is a number in that base. If base# is
omitted, then base 10 is used. When specifying n, if a non-digit
is required, the digits greater than 9 are represented by the
lowercase letters, the uppercase letters, @, and _, in that order.
If base is less than or equal to 36, lowercase and uppercase
letters may be used interchangeably to represent numbers between
10 and 35.

... which I guess looks like this:

bash-3.2$ echo $((0xff+1))
256
bash-3.2$ echo $((0Xff+1))
256
bash-3.2$ echo $((0377+1))
256
bash-3.2$ echo $((10#1+1))
2
bash-3.2$ echo $((16#ff+1))
256
bash-3.2$ echo $((2#11111111+1))
256

Happy to update the commit to match that syntax.

[mvdan]

Yep, mimicking bash is good. Just add some tests near existing arithmetic ones.

[stephen-fox]

@mvdan, I updated the commit to use bash's syntax for integer base handling. I am not a huge fan of the code I wrote, but I think it's a good starting point. I'm not sure if the test format is what you are looking for. But, the tests pass and the bash examples I posted earlier work now.

[mvdan]

Thanks! SGTM, just two thoughts.

[mvdan]

no need to quote the whole section of the bash man page; simply mention that this mimics the documented bash behavior.

[stephen-fox]

I removed it.

[mvdan]

no need to add an entirely new test; you can add test cases in interp/interp_test.go after the existing arithmetic ones with echo $((...)). An advantage of that route is that those get run against bash as well, so we catch any discrepancy.

[stephen-fox]

I moved the tests around as requested.

[mvdan]

Also, please add "Fixes #339" to the commit message.

[stephen-fox]

Hey @mvdan, I have updated the commit with the requested changes and ran the interp package's tests on a Debian machine and they passed.

[mvdan]

Thanks very much!

[mvdan]

Hm, the tests failed on 32-bit.

[stephen-fox]

Awesome. I can probably make the test pass by just using a smaller number... It does make me wonder about sh's handling of integer bit width and what the "correct" behavior would look like though.

[mvdan]

All the arithmetic code should be on 64-bit integers; there might be a spot that isn't the case. This might be a latent bug and not introduced by your fix, but perhaps just surfaced by your test.

[mvdan]

Ah, I see that expand.Arithm returns int rather than int64, and I have a TODO for that. Perhaps just comment out the test case you have here and add a smaller one for now. I'll deal with the int vs int64 issue at a later time.

[stephen-fox]

I commented out that test and added another with a smaller number.