Harmony is/isnt operators

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: evilpie, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

6 years ago
Assignee: general → evilpies
(Reporter)

Comment 1

6 years ago
Created attachment 585943 [details] [diff] [review]
WIP

This mostly works! Yaay. I am not sure about the parsing side, but it seems to work. jsop.tbl wants a precedence, but I have no idea how to come up with that number, for the moment I just coped JSOP_EQ.
(Reporter)

Comment 2

6 years ago
Okay, I just made some test, and for example this:

  (a == 
    0) is 1;

Doesn't work, but from my understanding it should. I hope, I don't need to do some deeper change to the parser, would appreciate some pointers :)
That while loop's body logic looks wrong. You want if ... else if ... else break.

When checking line numbers, beware bogus pn_pos.end values (latent bugs). Check parenthesized expression parsing. What values do you see in GDB for the two lines?

/be
(Reporter)

Comment 4

6 years ago
Created attachment 586029 [details] [diff] [review]
WIP v1

Yep Brendan you are right. Unless there is some other way to do this, we need to fix the position for every token. In some simple cases this already works.
Suggest using const ref to consolidate tokenStream.currentToken() repetition.

No paren-expression end fix?

/be
Most  nodes have correct end cords -- we should test and fix the ones that do not. No fear.

/be
(Reporter)

Comment 7

6 years ago
>No paren-expression end fix?
What?

>Most  nodes have correct end cords -- we should test and fix the ones that do not. No fear.
Good idea, indeed.
(In reply to Tom Schuster (evilpie) from comment #7)
> >No paren-expression end fix?
> What?

The end coord for a parenthesized expression used to be correct. The needed assignment was inadvertently lost by the cset landed for bug 461269 (cc'ing jorendorff). Here's a patch:

diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -7154,16 +7154,17 @@ Parser::primaryExpr(TokenKind tt, JSBool
         JSBool genexp;
 
         pn = parenExpr(&genexp);
         if (!pn)
             return NULL;
         pn->setInParens(true);
         if (!genexp)
             MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN);
+        pn->pn_pos.end = tokenStream.currentToken().pos.end;
         break;
       }
 
       case TOK_STRING:
         pn = atomNode(PNK_STRING, JSOP_STRING);
         if (!pn)
             return NULL;
         break;

Ugh, JSBool! Feel free to make a less minimal fix :-).

/be
(Reporter)

Comment 9

6 years ago
Created attachment 586587 [details] [diff] [review]
v1

So, I introduced a slight change in behavior, but only the warning messages for generators with no closing ), in a switch, with etc. changes, so not really important.
Attachment #585943 - Attachment is obsolete: true
Attachment #586029 - Attachment is obsolete: true
Attachment #586587 - Flags: review?(jorendorff)
(Reporter)

Comment 10

6 years ago
Created attachment 586591 [details] [diff] [review]
v1.1

Sorry made some small mistake, and confused begin and end!
Attachment #586587 - Attachment is obsolete: true
Attachment #586587 - Flags: review?(jorendorff)
Why are you pushing out the isGenerator parameter so a number (three?) of call sites of parenExpr need to declare and pass an utterly useless outparam?

Note also C++ mutable ref out params are bad style.

Really, this patch should be more minimal. Now there are two assignments to pn_pos.end depending on whether the paren-expr is a genexp, and widely separated at that. Yuck.

/be
(Reporter)

Comment 12

6 years ago
Created attachment 586601 [details] [diff] [review]
v2

Fixed that, have the review as you obviously care about this.
Attachment #586591 - Attachment is obsolete: true
Attachment #586601 - Flags: review?(brendan)
(Reporter)

Comment 13

6 years ago
Try run doesn't look very promising
https://tbpl.mozilla.org/?tree=Try&rev=ac069f589e9d

Okay something that I expected, but didn't really think of any actual testcase.
I thought that the case where, I currently throw always throws.
Some of other test, do the following:

>a++
>is(b);

We try to parse this as [a++] is (b);

To fix this wee need to rewrite to:
>if (TokenIsHarmonyEquality(tokenStream.currentToken(), context) &&
>    left->pn_pos.end.lineno == tokenStream.currentToken().pos.begin.lineno) {
(Reporter)

Updated

6 years ago
Attachment #586601 - Attachment is obsolete: true
Attachment #586601 - Flags: review?(brendan)
(Reporter)

Updated

6 years ago
Attachment #588905 - Flags: review?(brendan)
I can't parse comment 13. Tom, what needs to happen to move this forward?

/be
(Reporter)

Comment 16

6 years ago
This should actually just be ready now.
(Reporter)

Updated

6 years ago
Attachment #588905 - Flags: review?(brendan) → review?(jorendorff)
I apologize for not getting to this review this week. First thing Monday.
Comment on attachment 588905 [details] [diff] [review]
v3

Lovely patch.

Let's hold off on landing this. I have concerns about this language feature. I'm discussing them with Dave Herman and Brendan.

>+bool
>+TokenIsHarmonyEquality(const Token &token, JSContext *cx)
>+{
>+    if (token.type != TOK_NAME)
>+        return false;
>+    if (token.name() == cx->runtime->atomState.isAtom)
>+        return true;
>+    if (token.name() == cx->runtime->atomState.isntAtom)
>+        return true;
>+    return false;
>+}

This looks like a one-liner to me:

    return token.type == TOK_NAME &&
           (token.name() == cx->runtime->atomState.isAtom ||
            token.name() == cx->runtime->atomState.isntAtom);

> END_CASE(JSOP_NE)
> 
>+
>+
> #undef EQUALITY_OP

The style rule in js/src is, at most one blank line at a time. Please remove the extras.

>+#define SAME_VALUE_OP(OP, COND)                                             \
>+    JS_BEGIN_MACRO                                                          \
>+        const Value &rref = regs.sp[-1];                                    \
>+        const Value &lref = regs.sp[-2];                                    \
>+        JSBool equal;                                                       \
>+        if (!SameValue(cx, lref, rref, &equal))                             \
>+            goto error;                                                     \
>+        COND = equal OP JS_TRUE;                                            \
>+        regs.sp--;                                                          \
>+    JS_END_MACRO
>+
>+BEGIN_CASE(JSOP_IS)
>+{
>+    bool cond;
>+    SAME_VALUE_OP(==, cond);
>+    regs.sp[-1].setBoolean(cond);
>+}
>+END_CASE(JSOP_IS)

Macros are kind of yucky, and comparing a JSBool to JS_TRUE with == is definitely yucky. How about:

BEGIN_CASE(JSOP_IS)
{
    JSBool equal;
    if (!SameValue(cx, regs.sp[-1], regs.sp[-2], &equal))
        goto error;
    regs.sp--;
    regs.sp[-1].setBoolean(equal);
}
END_CASE(JSOP_IS)

And the same for JSOP_ISNT; the code duplication is only two lines more without the ten-line macro than with it, so it seems clear the macro isn't carrying its weight.

Please don't make a test directory named "harmony"; "es6" or else "is-isnt" would be fine. (I set aside a whole directory for "for-of"

>+assertEq(0 is 0, true);
>+assertEq(0 is -0, false);
>+assertEq(-0 is 0, false);

Add the fourth corner of the square:
  assertEq(-0 is -0, true);


>+function throws(code, expected) {

You can
  load(extdir + "asserts.js");
  assertThrowsInstanceOf(function () { eval("0\n is 1"); }, SyntaxError);

Please add a test or two that checks the operator precedence.

This also needs a few Reflect.parse tests.

And decompiler tests.

We may also want a pref so that we can disable this feature; let me ask around and see what we're doing about this.
Attachment #588905 - Flags: review?(jorendorff)
(Reporter)

Comment 19

6 years ago
I doubt this will ever happen.
Assignee: evilpies → general
TC39 needs to decide, but sentiment was against adding these operators. Object.is (or Object.sameValue -- better name?) should happen, though. It's "just API" and can be polyfilled. Is that this bug or another one?

/be

Comment 21

5 years ago
Closing based on meeting notes [1]
bug 839979 is the new place to be

[1] https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-29.md#41-isisnt-operators
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.