1221716 - Make BytecodeEmitter::emitTree more consistent about using the ok variable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Make the simpler parts of BytecodeEmitter::emitTree() use the `ok` boolean consistently

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Factor out all remaining complex cases from the switch statement in emitTree()

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Jason Orendorff [:jorendorff]]

(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.

Comment 6

[Jason Orendorff [:jorendorff]]

(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.

Comment 7

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b3a1bb6934

Comment 8

[Jason Orendorff [:jorendorff]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3fa07bb22f0a
https://hg.mozilla.org/mozilla-central/rev/774d4f99719a