Closed
Bug 1221716
Opened 7 years ago
Closed 7 years ago
Make BytecodeEmitter::emitTree more consistent about using the ok variable
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(2 files)
8.11 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
21.77 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
It always kind of bugged me how arbitrary it is - some cases assign to ok, some don't. Generally I dislike ok variables, but in this case it's OK.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8683287 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8683288 -
Flags: review?(jwalden+bmo)
Comment 3•7 years ago
|
||
Comment on attachment 8683287 [details] [diff] [review] Part 1: Make the simpler parts of BytecodeEmitter::emitTree() use the `ok` boolean consistently Review of attachment 8683287 [details] [diff] [review]: ----------------------------------------------------------------- Ugh. I mean, okay, but. I'd much rather we just killed off |ok| entirely, than extended it everywhere. It's one thing to use |ok| when you must, because there are cleanup actions that must be performed. But when it's entirely gratuitous, just to save a few |if (!) return false;|, it feels like too much to me.
Attachment #8683287 -
Flags: review?(jwalden+bmo) → review+
Comment 4•7 years ago
|
||
Comment on attachment 8683288 [details] [diff] [review] Part 2: Factor out all remaining complex cases from the switch statement in emitTree() Review of attachment 8683288 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +6809,5 @@ > return true; > } > > bool > +BytecodeEmitter::emitRightAssociative(ParseNode* pn) { { on same line, wut @@ +6810,5 @@ > } > > bool > +BytecodeEmitter::emitRightAssociative(ParseNode* pn) { > + MOZ_ASSERT(pn->isArity(PN_LIST)); MOZ_ASSERT(pn->isKind(PNK_POW)) surely? Heck, you should just name this emitExponentiation or something, since that's all it's good for. o/~ What is it good for? Exponentiation! (say it again) o/~ @@ +6839,5 @@ > + if (!emitTree(subexpr)) > + return false; > + if (!emit1(op)) > + return false; > + } if (!emitTree(pn->pn_head)) return false; JSOp op = pn->getOp(); ParseNode* nextExpr = pn->pn_head->pn_next; do { if (!emitTree(nextExpr)) return false; if (!emit1(op)) return false; } while ((nextExpr = nextExpr->pn_next)); feels slightly more readable to me, and also in its implicitly demanding that every left-associative list have at least two members. I don't think you need to call out avoiding recursion here -- this purely consumes a data structure, only structure generators should talk about that. @@ +6898,5 @@ > return true; > } > > +bool > +BytecodeEmitter::emitSequenceExpr(ParseNode* pn) Would prefer emitCommaExpression to this name, seems more understandable, less jargon-y (and not even ECMAscript jargon, really!) to me.
Attachment #8683288 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) > Ugh. I mean, okay, but. I'd much rather we just killed off |ok| entirely, > than extended it everywhere. OK, done. It's trivial enough either way.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > > +BytecodeEmitter::emitRightAssociative(ParseNode* pn) { > > { on same line, wut lol, fixed. > > bool > > +BytecodeEmitter::emitRightAssociative(ParseNode* pn) { > > + MOZ_ASSERT(pn->isArity(PN_LIST)); > > MOZ_ASSERT(pn->isKind(PNK_POW)) surely? Yes, added. > Heck, you should just name this > emitExponentiation or something, since that's all it's good for. Hmm. Yes, but I like the effect in emitTree(): ... case PNK_URSH: case PNK_STAR: case PNK_DIV: case PNK_MOD: if (!emitLeftAssociative(pn)) return false; break; case PNK_POW: if (!emitRightAssociative(pn)) return false; break; "Wait, why does (**) get its own separate case? Oh, it's right-associative." The method name says why you're calling it. Even better than saying what it does, imho, but of course that's a style thing... > [...] > feels slightly more readable to me, and also in its implicitly demanding > that every left-associative list have at least two members. Mmm, I disagree with this but have no desire to discuss it so I took the proposed change :) > I don't think > you need to call out avoiding recursion here -- this purely consumes a data > structure, only structure generators should talk about that. Done. > > +bool > > +BytecodeEmitter::emitSequenceExpr(ParseNode* pn) > > Would prefer emitCommaExpression to this name, seems more understandable, > less jargon-y (and not even ECMAscript jargon, really!) to me. Done.
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b3a1bb6934
Assignee | ||
Comment 8•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa07bb22f0afcce7250ea3dd82d4e57a847336e Bug 1221716 - Part 1: Make the simpler parts of BytecodeEmitter::emitTree() use the `ok` boolean consistently. r=Waldo. https://hg.mozilla.org/integration/mozilla-inbound/rev/774d4f99719af3299c699c9f8918f53975121adc Bug 1221716 - Part 2: Factor out all remaining complex cases from the switch statement in emitTree(). r=Waldo.
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3fa07bb22f0a https://hg.mozilla.org/mozilla-central/rev/774d4f99719a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•