`JS_NewFloat64` is a misleading name

[chqrlie]

JS_NewFloat64 tests if its double argument is an int32_t and if so creates a JS_TAG_INT value instead of a JS_TAG_FLOAT64 value.

This is somewhat misleading and prevents creation of actual JS_TAG_FLOAT64 values in places where it would be more appropriate than JS_TAG_INT values, such as Map keys. There is a __JS_NewFloat64 for this but it is debatable whether it should be documented if published at all.

I propose we add a JS_NewNumber API that keeps the current heuristics, change JS_NewFloat64 to always create JS_TAG_FLOAT64 values and remove __JS_NewFloat64.

Note also that float_is_int32 returns false for INT32_MIN. I did some benchmarking for various alternatives to implement this test and it seems a much simpler test code is at least as efficient and much more readable, portable and reliable:

static BOOL float_is_int32(double d)
{
    JSFloat64Union t1, t2;

    if (d >= INT32_MIN && d <= INT32_MAX) {
        t1.d = d;
        t2.d = (int32_t)d;
        return t1.u64 == t2.u64;
    } else {
        return FALSE;
    }
}

[saghul]

Sound good. Out of curiosity, why would a float be preferred as map keys?

[chqrlie]

Sound good. Out of curiosity, why would a float be preferred as map keys?

Because comparing values with identical numeric values is more efficient if the stored types are the same.

Map hash keys must be identical for JS_TAG_INT values and equivalent JS_TAG_FLOAT64 values. The current implementation only normalizes -0.0 as 0 keys but it would be more efficient if it also converted integer numeric keys to float64 values so the conversion would only be performed once per lookup and insertion and never needed for rehashing.

[saghul]

Thanks for explaining! Sounds like a good plan to me.

[chqrlie]

I have a patch almost ready to get rid of __JS_NewFloat64, make JS_NewFloat64 always create a floating point value and add JS_NewDouble to create an integer if possible. I am not satisfied with this name, maybe JS_NewNumber would be better?

[saghul]

JS_NewNumber sounds good to me!

[chqrlie]

Fixed by #319