[tarantool-patches] Re: [PATCH v1 1/1] lua: fix strange behaviour of tonumber64

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Alexander Turenko <alexander.turenko@xxxxxxxxxxxxx>
  • Date: Mon, 16 Jul 2018 16:15:39 +0300



On 16/07/2018 15:49, Alexander Turenko wrote:

Hi, Vlad!

That is interesting discussion. Hope you don't mind my participation.

Hi! Your participation is appreciated!


WBR, Alexander Turenko.

On Mon, Jul 16, 2018 at 01:23:36PM +0300, Vladislav Shpilevoy wrote:
Thanks for the patch! See 4 comments below.

On 13/07/2018 14:21, Kirill Shcherbatov wrote:
Function tonumber64 has worked incorrectly with values less
than LLONG_MIN.
Now it works in the interval [LLONG_MIN, ULLONG_MAX] returning
nil otherwise.

Closes #3466.
---
Branch: 
https://github.com/tarantool/tarantool/compare/kshch/gh-3466-tonumber64-strange-behaviour
Issue: https://github.com/tarantool/tarantool/issues/3466

   src/lua/init.c         |  6 +++++-
   test/box/misc.result   | 19 +++++++++++++++++++
   test/box/misc.test.lua |  8 ++++++++
   3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/src/lua/init.c b/src/lua/init.c
index 9a96030..4b5285d 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -222,7 +222,11 @@ lbox_tonumber64(struct lua_State *L)
                        if (argl == 0) {
                                lua_pushnil(L);
                        } else if (negative) {
-                               luaL_pushint64(L, -1 * (long long )result);
+                               if (result > -((unsigned long long )LLONG_MIN)) 
{

1. Please, do not enclose one-line bodies into {}.

2. How can you cast LLONG_MIN (that is negative) to the unsigned type?


Cast does not change bits. It is legal.

Yes, technically it is legal, but casting negative value to an unsigned type
looks weird.


3. Why not 'result > LLONG_MAX'? As I understand, abs(LLONG_MAX) == 
abs(LLONG_MIN),
it is not? (http://www.cplusplus.com/reference/climits/)


No, LLONG_MAX is 2^63-1, but LLONG_MIN is -2^63. We want to compare
result with 2^63. We are trying to do so in platform-independent way
(hovewer unsiged unary nimus equivalence with signed one is likely
two-complement number representation property and can be violated on
other platforms).

Are you think we should introduce our own constant
9223372036854775808ULL (2^63) and avoid that complex assumptions set? It

Ultimately no. We should not invent the constants.

would be explicitly number-representation-dependent, so maybe it is
better.

Ok. Logically we want an error on -result < INT64_MIN, right?
It is the same as result > -INT64_MIN. But we can not say
-INT64_MIN because abs(INT64_MIN) > INT64_MAX, yes?

Then lets rephrase the comparison:

    result > -INT64_MIN
           |
           v
  result + 1 >= -INT64_MIN
           |
           v
    result >= -INT64_MIN - 1
           |
           v
   result >= -(INT64_MIN + 1) <- that is the solution.

As I understand, -(INT64_MIN + 1) is exactly 2^63 - 1 and
fits in int64, right?

Other related posts: