515285 - JS_SameValue

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Use it in assertEq, prep it for use in ES5 work, write some jsapi-tests for it (since the current implementation, used there, isn't public, and since it has a bug in it :-\ ).

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Bug is demonstrated by the test -- JS_NewDoubleValue returns a double for +0, not JSVAL_ZERO.

I ran the test suite, and no tests need to be updated for the assertEq change.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 399379 [details] [diff] [review]
Patch

>+static inline bool
>+IsNegativeZero(jsval v)
>+{
>+    if (!JSVAL_IS_DOUBLE(v))
>+        return false;
>+    return *JSVAL_TO_DOUBLE(v) == 0 && signbit(*JSVAL_TO_DOUBLE(v)) == 1;
>+}

Use JSDOUBLE_IS_NEGZERO.

>+static inline bool
>+IsNaN(jsval v)
>+{
>+    if (!JSVAL_IS_DOUBLE(v))
>+        return false;
>+    jsdouble d = *JSVAL_TO_DOUBLE(v);
>+    return d != d;

MSVC botches this, or used to. Better to use JSDOUBLE_IS_NaN and let _isnan do the work (it may be an intrinsic, I don't know; but it's what the rest of the engine uses).

>+JSBool
>+js_SameValue(jsval v1, jsval v2, JSContext *cx)
>+{
>+    if (IsNegativeZero(v1))
>+        return IsNegativeZero(v2);
>+    if (IsNegativeZero(v2))
>+        return IsNegativeZero(v1);
>+    if (IsNegativeZero(v1) || IsNegativeZero(v2))
>+        return JS_FALSE;

This function already tested IsNegativeZero(v1) first thing, and returned something if so. Ditto IsNegativeZero(v2). So neither can be true here, so this is dead code.

Can gcc CSE all the JSVAL_IS_DOUBLE(v) tests and *JSVAL_IS_DOUBLE(v) loads? It ought to be able to.

/be

Comment 3

[Jeff Walden [:Waldo]]

Attachment: v2

(In reply to comment #2)
> This function already tested IsNegativeZero(v1) first thing, and returned
> something if so. Ditto IsNegativeZero(v2). So neither can be true here, so this
> is dead code.

Yikes, how'd I manage that?


> Can gcc CSE all the JSVAL_IS_DOUBLE(v) tests and *JSVAL_IS_DOUBLE(v) loads? It
> ought to be able to.

Not worth worrying about until it shows up on a profile, I think.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > Can gcc CSE all the JSVAL_IS_DOUBLE(v) tests and *JSVAL_IS_DOUBLE(v) loads? It
> > ought to be able to.
> 
> Not worth worrying about until it shows up on a profile, I think.

Check the generated code. We should do that as low-level VM hackers, as part of our work habits.

Agree it's cleaner this way. Indeed API users have asked about NaN checking, so maybe we should put the jsval-parameterized predicates in jsapi.h with ugly JS_ prefixes.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 399531 [details] [diff] [review]
v2

>diff --git a/js/src/jsapi-tests/selfTest.cpp b/js/src/jsapi-tests/selfTest.cpp
> BEGIN_TEST(selfTest_negativeZeroIsNotTheSameAsZero)

I would just delete this test. The functionality has moved from tests.h into js/src proper--moved, not copied--so it makes sense to me to move this test over to testSameValue.cpp, rather than copy.

> {
>     jsvalRoot negativeZero(cx);
>     EVAL("-0", negativeZero.addr());
>-    CHECK(!sameValue(negativeZero, JSVAL_ZERO));
>+    CHECK_DIFFERENT(negativeZero, JSVAL_ZERO);
>+
>+    jsvalRoot positiveZero(cx);

>+    CHECK(JS_NewDoubleValue(cx, 0.0, positiveZero.addr()));

Actually, calling JS_NewDouble or JS_NewDoubleValue with an integer value is considered API misuse. See the documentation or bug 435288.

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_NewDouble
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_NewDoubleValue

We haven't enforced it with an assertion for reasons detailed in that bug. :(

>+++ b/js/src/jsapi-tests/testSameValue.cpp
>+    CHECK(JS_NewDoubleValue(cx, 0.0, v1.addr()));

Also here.

(If you wanted to fix the problem in bug 435288's description in the opposite direction by fixing JSOP_TABLESWITCH, I wouldn't object. Separate bug of course.)

>+++ b/js/src/jsapi-tests/testSameValue.cpp
>+    CHECK(!JS_SameValue(cx, v1, v2));
>+    CHECK_DIFFERENT(v1, v2);

This seems redundant.

>+++ b/js/src/jsapi-tests/tests.h
>- * vim: set ts=8 sw=4 et tw=78:
>+ * vim: set ts=8 sw=4 et tw=99:

I could swear I saw brendan make exactly the opposite change someplace, and recently. Could you ask him about it, and maybe patch all our files at once to have correct mode lines... preferably in another bug?

>+#define CHECK_DIFFERENT(v1, v2) \
>+    do { \
>+        if (!checkDifferent(v1, v2, #v1, #v2, __FILE__, __LINE__)) \
>+            return false; \
>+    } while (false)
>+
>+    bool checkDifferent(jsval v1, jsval v2,
>+                        const char *v1Expr, const char *v2Expr,
>+                        const char *filename, int lineno) {
>+        return !JS_SameValue(cx, v1, v2) ||
>+            fail(std::string("CHECK_DIFFERENT failed: expected !JS_SameValue(cx, ") +
>+                 v1Expr + ", " + v2Expr + "), got JS_SameValue(cx, " +
>+                 toSource(v1) + ", " + toSource(v2) + ")", filename, lineno);
>+    }

Let's not do this. It's not generally useful, and if you take my recommendations above I don't think anything is left using it at all.

If you do need it, you can put it in the individual test or test*.cpp file that wants it.

IsNegativeZero:
>+    if (!JSVAL_IS_DOUBLE(v))
>+        return false;
>+    return JSDOUBLE_IS_NEGZERO(*JSVAL_TO_DOUBLE(v));

return JSVAL_IS_DOUBLE(v) && JSDOUBLE_IS_NEGZERO...?

IsNaN:
>+    if (!JSVAL_IS_DOUBLE(v))
>+        return false;
>+    return JSDOUBLE_IS_NaN(*JSVAL_TO_DOUBLE(v));

Same here.

>+
>+JSBool
>+js_SameValue(jsval v1, jsval v2, JSContext *cx)
>+{
>+    if (IsNegativeZero(v1))
>+        return IsNegativeZero(v2);
>+    if (IsNegativeZero(v2))
>+        return IsNegativeZero(v1);

return false; on that line

>+    if (IsNaN(v1) && IsNaN(v2))
>+        return JS_TRUE;

In this case are v1 and v2 both == DOUBLE_TO_JSVAL(rt->jsNaN)? I didn't bother looking when I wrote jsapi-tests because ...well, I wasn't in jsinterp.cpp, basically. But you could save some checking here, if so.

>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
> static JSBool
> AssertEq(JSContext *cx, uintN argc, jsval *vp)
> {
>-    if (argc != 2) {
>+    if (argc != 2 && (argc != 3 || !JSVAL_IS_STRING(JS_ARGV(cx, vp)[2]))) {

I like this wording better, because (I did not learn boolean algebra at an early age and) it's more obvious to me what's going on if I say what's allowed and then stick a ! on the front:

  if (!(argc == 2 || (argc == 3 && JSVAL_IS_STRING...))) {

Do you agree? I'm curious here -- when reading your original, I found myself mentally negating all the terms. But maybe it's just me.

>-    if (!JS_StrictlyEqual(cx, argv[0], argv[1])) {
>+    if (!JS_SameValue(cx, argv[0], argv[1])) {

Yay!

>+        const char *msg = argc == 3 ? JS_GetStringBytes(JSVAL_TO_STRING(argv[2])) : "";
>+        if (argc == 2) {
>+            JS_ReportErrorNumber(cx, my_GetErrorMessage, NULL, JSSMSG_ASSERT_EQ_FAILED,
>+                                 actual, expected);
>+        } else {
>+            JS_ReportErrorNumber(cx, my_GetErrorMessage, NULL, JSSMSG_ASSERT_EQ_FAILED_MSG,
>+                                 actual, expected, msg);
>+        }

Looks like the declaration of msg belongs in the else block.

r=me with all those fixes, and thanks for doing this!

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/b6151cd2c62c

(In reply to comment #5)
> I could swear I saw brendan make exactly the opposite change someplace, and
> recently. Could you ask him about it, and maybe patch all our files at once to
> have correct mode lines... preferably in another bug?

Filed bug 516055.


> Let's not do this. It's not generally useful, and if you take my
> recommendations above I don't think anything is left using it at all.

I think Mochitest's isnot suggests this would be useful and used, but punted for later.


> In this case are v1 and v2 both == DOUBLE_TO_JSVAL(rt->jsNaN)? I didn't bother
> looking when I wrote jsapi-tests because ...well, I wasn't in jsinterp.cpp,
> basically. But you could save some checking here, if so.

No, printing the two jsvals shows they have different values and therefore can't both equal DOUBLE_TO_JSVAL(rt->jsNaN).


> >diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp

>   if (!(argc == 2 || (argc == 3 && JSVAL_IS_STRING...))) {
> 
> Do you agree?

I don't really like either option very much, would almost rather |if (argc == 2 || (argc == 3 && isString(argv[2]))) realBody(); else fail();|.  I switched to yours since you care and because I can't quite justify my alternative.


> Looks like the declaration of msg belongs in the else block.

Done, got rid of the one-use temp as well.

Comment 7

[Jeff Walden [:Waldo]]

https://developer.mozilla.org/en/JS_SameValue

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/b6151cd2c62c

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8a5c8c08cd2d