Add `JsNumberToInt64` function #4827

kfarnung · 2018-03-14T23:27:14Z

Allow the caller to get the integer value for a given number back as a
64-bit integer value.

kfarnung · 2018-03-14T23:28:55Z

I had written this up when trying to fix a bug in node-chakracore, but ended up just using a static cast from double to int64_t instead. Looking to see if other people think this would be useful.

kfarnung · 2018-03-14T23:29:15Z

lib/Jsrt/Jsrt.cpp

+CHAKRA_API JsNumberToInt64(_In_ JsValueRef value, _Out_ int64_t *result)
+{
+    return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);


I should fix this before landing this PR

MSLaguana · 2018-03-14T23:29:50Z

Is this only guaranteed to be precise for 52 bit numbers?

kfarnung · 2018-03-14T23:51:45Z

It's still subject to the significant bit limits of the IEEE754 double-precision numbers. In this case it's 53-bits of integer precision (technically 54 if you count the sign bit).

I can add that remark to the documentation if there's agreement that the function is useful.

boingoing

Seems alright to me

kfarnung · 2018-03-15T00:24:48Z

@MSLaguana I updated the comment to call out the precision caveat.

kfarnung · 2018-03-15T02:06:30Z

@boingoing Can you take another look?

MSLaguana · 2018-03-15T02:14:33Z

lib/Jsrt/Jsrt.cpp

+        }
+        else if (Js::JavascriptNumber::Is_NoTaggedIntCheck(value))
+        {
+            *result = Js::JavascriptConversion::ToInt64(Js::JavascriptNumber::GetValue(value));


Does this case apply to Number objects, e.g. the result of new Number(1)? If so, this conversion can be re-entrant into script via a custom valueOf I think? If so, then you can't guarantee that there won't be an exception here, since the script code can do anything.

No, this is explicitly checking for the tagged number case/JavascriptNumber case. Number objects are JavascriptNumberObjects. That being said, should we be checking for JavascriptNumberObject here? It looks like the existing implementations of JsNumberTo* don't handle Number objects, they likely require the caller to call JsConvertValueToNumber first.

boingoing · 2018-03-15T02:15:55Z

lib/Runtime/Language/JavascriptConversion.cpp

@@ -1256,6 +1256,11 @@ namespace Js
        return JavascriptMath::ToInt32Core(T1);
    }

+    __int64 JavascriptConversion::ToInt64(double T1)
+    {
+        return JavascriptMath::TryToInt64(T1);


What should we do if NumberUtilities::IsValidTryToInt64 is false for the result of this call?

Seems like TryToInt64 would handle this case? In the case of overflows/Nans, it would just return canonical values? We should however document it

TryToInt64 doesn't do anything special for x64 builds, it's simply a cast from double to int64_t. There are special paths for x86 (which can use SSE3) and ARM (which has different behavior), but in the base case it's just a cast.

For ToInt32 it ends up normalizing all of these cases down to zero without returning an error, but I'm not sure if that's spec'd anywhere.

digitalinfinity

Generally LGTM

digitalinfinity · 2018-03-15T06:40:43Z

lib/Jsrt/Jsrt.cpp

+        }
+        else if (Js::JavascriptNumber::Is_NoTaggedIntCheck(value))
+        {
+            *result = Js::JavascriptConversion::ToInt64(Js::JavascriptNumber::GetValue(value));


No, this is explicitly checking for the tagged number case/JavascriptNumber case. Number objects are JavascriptNumberObjects. That being said, should we be checking for JavascriptNumberObject here? It looks like the existing implementations of JsNumberTo* don't handle Number objects, they likely require the caller to call JsConvertValueToNumber first.

digitalinfinity · 2018-03-15T06:44:26Z

lib/Runtime/Language/JavascriptConversion.cpp

@@ -1256,6 +1256,11 @@ namespace Js
        return JavascriptMath::ToInt32Core(T1);
    }

+    __int64 JavascriptConversion::ToInt64(double T1)
+    {
+        return JavascriptMath::TryToInt64(T1);


Seems like TryToInt64 would handle this case? In the case of overflows/Nans, it would just return canonical values? We should however document it

digitalinfinity · 2018-03-15T06:46:15Z

bin/NativeTests/JsRTApiTest.cpp

+        // Number.MIN_SAFE_INTEGER
+        REQUIRE(JsDoubleToNumber(-9007199254740991, &value) == JsNoError);
+        REQUIRE(JsNumberToInt(value, &intValue) == JsNoError);
+        CHECK(1 == intValue);


Mild preference for defining Pos_InvalidInt64/Neg_InvalidInt64 as JSRT constants, and compare against that?

In this case it's not actually invalid, it's the result of overflowing the int32, I'm still trying to figure out what to do for truly invalid numbers that exceed the bounds of int64, those end up returning 0x8000000000000000 on x86/x64, but that doesn't seem to be 100% consistent across architectures.

digitalinfinity · 2018-03-15T06:46:53Z

bin/NativeTests/JsRTApiTest.cpp

+        REQUIRE(JsNumberToInt(value, &intValue) == JsNoError);
+        CHECK(1 == intValue);
+        REQUIRE(JsNumberToInt64(value, &int64Value) == JsNoError);
+        CHECK(-9007199254740991ll == int64Value);



Can you add some test cases for Nan and overflow too?

IrinaYatsenko · 2018-03-15T15:03:25Z

lib/Jsrt/ChakraCore.h

@@ -1063,5 +1063,22 @@ CHAKRA_API
        _Out_opt_ JsValueRef* target,
        _Out_opt_ JsValueRef* handler);

+///     Retrieves the <c>int64_t</c> value of a number value.
+/// </summary>
+/// <remarks>


Nit: missing opening
tag

Thanks! I must have dropped it when I was rebasing.

akroshg

liminzhu · 2018-05-15T23:46:15Z

Just saw this PR. Btw there's a new primitive BigInt proposal in TC39 (stage 3 already, pretty close to standardized), which allows representing int64 in JS. It would complicate things a bit if this API also takes BigInt.

kfarnung · 2018-05-15T23:51:27Z

There are some open questions about how this API should handle numbers too large to represent even without considering BigInt. For Int32 we seem to just wrap back around, but for Int64 the default behavior is to return a number indicating the invalid state (some CPUs do this by returning 0x8000000000000000 and we appear to use that ourselves).

I suspect the same sort of issues will occur with BigInt, except that 0x8000000000000000 will be possible to represent exactly with a BitInt when it's not possible to do so using a double.

Any thoughts about what the API should do in cases where the number can't be represented in an Int64 value?

liminzhu · 2018-05-16T21:30:13Z

for Int64 the default behavior is to return a number indicating the invalid state

It would be good to study some prior art. Can you point me to what language/platform use this as a default behavior? I was thinking more along the lines of returning a overflow error code, since you'd be getting that from .NET if you try to convert a bigint too large to int64.

I looked a bit more into BigInt and it looks like the proposal champion wants to set a clear boundary between BigInt and Number by having them as separate primitives and disallowing implicit conversions and ops mix-up, which is an approach I can agree with. This also makes me feel that if we were to expose BigInt natively, we want a different API, and b/c there is no builtin in C corresponding directly to bigint, we'll have to think about that later.

Allow the caller to get the integer value for a given number back as a 64-bit integer value.

alexgwolff · 2019-04-14T21:45:37Z

hey, what is the final decision on this PR? I need to convert a Int64 in .net environment.
thanks

alexgwolff · 2019-04-25T11:39:38Z

@kfarnung ??

kfarnung · 2019-04-25T21:51:36Z

@alexgwolff technically you can just convert the number to a double and then cast to a 64-bit integer. Since JavaScript numbers are all IEEE754, that's essentially what this code does internally.

The problem was that we couldn't find agreement on how to represent invalid conversions, that behavior isn't defined by the standard and V8 seemed to use the value that the base conversion returns (0x8000000000000000), but that's basically an architecture-specific magic number.

kfarnung commented Mar 14, 2018

View reviewed changes

boingoing approved these changes Mar 15, 2018

View reviewed changes

kfarnung force-pushed the jsrtint64 branch from 07112cf to 99cb16f Compare March 15, 2018 00:24

kfarnung self-assigned this Mar 15, 2018

MSLaguana reviewed Mar 15, 2018

View reviewed changes

boingoing reviewed Mar 15, 2018

View reviewed changes

digitalinfinity approved these changes Mar 15, 2018

View reviewed changes

IrinaYatsenko reviewed Mar 15, 2018

View reviewed changes

akroshg approved these changes Apr 13, 2018

View reviewed changes

kfarnung force-pushed the jsrtint64 branch from aad2e36 to bea9ac2 Compare May 16, 2018 00:17

kfarnung force-pushed the jsrtint64 branch from bea9ac2 to 52e51da Compare June 27, 2018 22:23

kfarnung changed the base branch from release/1.9 to release/1.10 June 27, 2018 22:24

kfarnung changed the base branch from release/1.10 to master July 27, 2018 21:15

Add JsNumberToInt64 function

e4ca8a5

Allow the caller to get the integer value for a given number back as a 64-bit integer value.

kfarnung force-pushed the jsrtint64 branch from 52e51da to e4ca8a5 Compare July 27, 2018 21:40

tondrej mentioned this pull request Mar 31, 2019

How to operate JsNumberToInt64?! tondrej/chakracore-delphi#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `JsNumberToInt64` function #4827

Add `JsNumberToInt64` function #4827

kfarnung commented Mar 14, 2018

kfarnung commented Mar 14, 2018

kfarnung Mar 14, 2018

MSLaguana commented Mar 14, 2018

kfarnung commented Mar 14, 2018 •

edited

boingoing left a comment

kfarnung commented Mar 15, 2018

kfarnung commented Mar 15, 2018

MSLaguana Mar 15, 2018 •

edited

digitalinfinity Mar 15, 2018

boingoing Mar 15, 2018

digitalinfinity Mar 15, 2018

kfarnung Mar 16, 2018

digitalinfinity left a comment

digitalinfinity Mar 15, 2018

digitalinfinity Mar 15, 2018

digitalinfinity Mar 15, 2018

kfarnung Mar 16, 2018

digitalinfinity Mar 15, 2018

IrinaYatsenko Mar 15, 2018

kfarnung Mar 15, 2018

akroshg left a comment

liminzhu commented May 15, 2018

kfarnung commented May 15, 2018

liminzhu commented May 16, 2018

alexgwolff commented Apr 14, 2019 •

edited

alexgwolff commented Apr 25, 2019

kfarnung commented Apr 25, 2019

Add JsNumberToInt64 function #4827

Are you sure you want to change the base?

Add JsNumberToInt64 function #4827

Conversation

kfarnung commented Mar 14, 2018

kfarnung commented Mar 14, 2018

Choose a reason for hiding this comment

MSLaguana commented Mar 14, 2018

kfarnung commented Mar 14, 2018 • edited

boingoing left a comment

Choose a reason for hiding this comment

kfarnung commented Mar 15, 2018

kfarnung commented Mar 15, 2018

MSLaguana Mar 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalinfinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg left a comment

Choose a reason for hiding this comment

liminzhu commented May 15, 2018

kfarnung commented May 15, 2018

liminzhu commented May 16, 2018

alexgwolff commented Apr 14, 2019 • edited

alexgwolff commented Apr 25, 2019

kfarnung commented Apr 25, 2019

Add `JsNumberToInt64` function #4827

Add `JsNumberToInt64` function #4827

kfarnung commented Mar 14, 2018 •

edited

MSLaguana Mar 15, 2018 •

edited

alexgwolff commented Apr 14, 2019 •

edited