775391 - Fix potential erroneous comment in DefineArg

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eddy Bruel [:ejpbruel]]

The comment in http://mxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#900 doesn't make sense. We assert that pnarg->op == JSOP_NOP before calling Define (causing pnarg to be added to tc->decls.), and before appending pnarg to pn->pn_body. Neither of those operations check the opcode of pnarg, nor do they perform a clone of pnarg. Yet immediately after we set pnarg->pn_op to JSOP_GETARG.

It seems to me that that comment is wrong, and we should be able to just set pnarg->op to JSOP_GETARG from the start.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

I've tested this patch with jit-tests and jstests and didn't find any regressions.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 643704 [details] [diff] [review]
Patch to be reviewed

Review of attachment 643704 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +919,5 @@
>      ParseNode *argpn = NameNode::create(PNK_NAME, atom, parser, parser->tc);
>      if (!argpn)
>          return false;
> +    argpn->setOp(JSOP_GETARG);
> +    JS_ASSERT(argpn->isKind(PNK_NAME) && argpn->isOp(JSOP_GETARG));

In the same way we don't write this:

  int x = 3;
  JS_ASSERT(x == 3);

I think this assertion can now be removed :)

Comment 3

[André Bargull [:anba]]

Fixed in http://hg.mozilla.org/mozilla-central/rev/d1b17b12a472