Closed
Bug 715359
Opened 13 years ago
Closed 11 years ago
Harmony is/isnt operators
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: evilpie, Unassigned)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 5 obsolete files)
16.55 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•13 years ago
|
Assignee: general → evilpies
Reporter | ||
Comment 1•13 years ago
|
||
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•13 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 :)
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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.
Comment 5•13 years ago
|
||
Suggest using const ref to consolidate tokenStream.currentToken() repetition. No paren-expression end fix? /be
Comment 6•13 years ago
|
||
Most nodes have correct end cords -- we should test and fix the ones that do not. No fear. /be
Reporter | ||
Comment 7•13 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.
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
Sorry made some small mistake, and confused begin and end!
Attachment #586587 -
Attachment is obsolete: true
Attachment #586587 -
Flags: review?(jorendorff)
Comment 11•13 years ago
|
||
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•13 years ago
|
||
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•13 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 | ||
Comment 14•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #586601 -
Attachment is obsolete: true
Attachment #586601 -
Flags: review?(brendan)
Reporter | ||
Updated•12 years ago
|
Attachment #588905 -
Flags: review?(brendan)
Comment 15•12 years ago
|
||
I can't parse comment 13. Tom, what needs to happen to move this forward? /be
Reporter | ||
Comment 16•12 years ago
|
||
This should actually just be ready now.
Reporter | ||
Updated•12 years ago
|
Attachment #588905 -
Flags: review?(brendan) → review?(jorendorff)
Comment 17•12 years ago
|
||
I apologize for not getting to this review this week. First thing Monday.
Comment 18•12 years ago
|
||
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)
Comment 20•12 years ago
|
||
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•11 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
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•