670049 - Start adding accessors for pn_* members of JSParseNode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: The first shade of lipstick for Ms. Parser.

Just a mechanical change. This patch let me break on things like setOp. It also found one assert that was actually a mutation!

For a followup it'll be super useful to have accessors for the parse node union members, since the macro definitions are not available to GDB.

Comment 1

[Jeff Walden [:Waldo]]

Newsletter, kplzthx.

Comment 2

[Dave Herman [:dherman]]

Comment on attachment 544661 [details] [diff] [review]
The first shade of lipstick for Ms. Parser.

Mostly nits, and just a couple minor things I didn't understand.

>-                    EMIT_UINT16_IMM_OP((PN_OP(pn2) == JSOP_SETARG)
>+                    EMIT_UINT16_IMM_OP((pn2->isOp(JSOP_SETARG))

Overparenthesized.

>-                JS_ASSERT(pn3->pn_type == TOK_NAME ||
>-                          pn3->pn_type == TOK_STRING);
>+                JS_ASSERT(pn3->isKind(TOK_NAME) ||
>+                          pn3->isKind(TOK_STRING));

This would fit on one line.

>-                    pn2->pn_op = uint8(op);
>+                    pn2->setOp(op);

I don't understand why the uint8 cast was here, or why you're eliminating it.

>-            JS_ASSERT(pn->pn_arity == PN_NULLARY);
>-            ok = (pn->pn_op == JSOP_OBJECT)
>-                 ? EmitObjectOp(cx, pn->pn_objbox, PN_OP(pn), cg)
>-                 : EmitAtomOp(cx, pn, PN_OP(pn), cg);
>+            JS_ASSERT(pn->isArity(PN_NULLARY));
>+            ok = (pn->isOp(JSOP_OBJECT))

Overparenthesized.

>-        JSFunctionBox *funbox = (pn->pn_type == TOK_FUNCTION) ? pn->pn_funbox : NULL;
>+        JSFunctionBox *funbox = (pn->isKind(TOK_FUNCTION)) ? pn->pn_funbox : NULL;

Overparenthesized.

>-            JS_ASSERT(pn->pn_type == TOK_SEMI ||
>-                      pn->pn_op == JSOP_LEAVEBLOCKEXPR);
>+            JS_ASSERT(pn->isKind(TOK_SEMI) ||
>+                      pn->isOp(JSOP_LEAVEBLOCKEXPR));

This would fit on one line.

>-            pn2->pn_op = (PN_OP(pn2) == JSOP_ARGUMENTS)
>-                         ? JSOP_SETNAME
>-                         : (pn2->pn_dflags & PND_BOUND)
>-                         ? JSOP_SETLOCAL
>-                         : (data.op == JSOP_DEFCONST)
>-                         ? JSOP_SETCONST
>-                         : JSOP_SETNAME;
>+            pn2->setOp((pn2->isOp(JSOP_ARGUMENTS))

Overparenthesized.

>       case TOK_RC:
>-        return (pn_op == JSOP_NEWINIT) && !(pn_xflags & PNX_NONCONST);
>+        return (isOp(JSOP_NEWINIT)) && !(pn_xflags & PNX_NONCONST);

Overparenthesized.

>-                    if (cond == (pn->pn_type == TOK_OR)) {
>+                    if (cond == (pn->isKind(TOK_OR))) {

Overparenthesized.

>-                        JS_ASSERT(cond == (pn->pn_type == TOK_AND));
>+                        JS_ASSERT(cond == (pn->isKind(TOK_AND)));

Overparenthesized.

>-                if (cond == (pn->pn_type == TOK_OR)) {
>+                if (cond == (pn->isKind(TOK_OR))) {

Overparenthesized.

>-                    JS_ASSERT(cond == (pn->pn_type == TOK_AND));
>+                    JS_ASSERT(cond == (pn->isKind(TOK_AND)));

Overparenthesized.

>-#define PN_OP(pn)    ((JSOp)(pn)->pn_op)
>-#define PN_TYPE(pn)  ((js::TokenKind)(pn)->pn_type)
>+  public:
>+    JSOp getOp() const                     { return JSOp(pn_op); }
>+    void setOp(JSOp op)                    { pn_op = op; }
>+    bool isOp(JSOp op) const               { return getOp() == op; }
>+    js::TokenKind getKind() const          { return js::TokenKind(pn_type); }
>+    void setKind(js::TokenKind kind)       { pn_type = kind; }
>+    bool isKind(js::TokenKind kind) const  { return getKind() == kind; }
>+    JSParseNodeArity getArity() const      { return JSParseNodeArity(pn_arity); }
>+    bool isArity(JSParseNodeArity a) const { return getArity() == a; }
>+    void setArity(JSParseNodeArity a)      { pn_arity = a; }
>+    /* Boolean attributes. */
>+    bool isInParens() const                { return pn_parens; }
>+    void setInParens(bool enabled)         { pn_parens = enabled; }
>+    bool isDefn() const                    { return pn_defn; }
>+    void setDefn(bool enabled)             { pn_defn = enabled; }
>+    bool isUsed() const                    { return pn_used; }
>+    void setUsed(bool enabled)             { pn_used = enabled; }

Looks nice.

>-    // Grr, windows.h or something under it #defines CONST...
>-#ifdef CONST
>-# undef CONST
>-#endif

Mmm, much less grumbly. I'm assuming this is tested and works in Windows (has windows.h changed?).

>-    return (PN_TYPE(pn) == TOK_VAR)
>+    return (pn->isKind(TOK_VAR))
>            ? variableDeclaration(pn, false, dst)
>-           : (PN_TYPE(pn) == TOK_LET)
>+           : (pn->isKind(TOK_LET))

Overparenthesized.

>         return expression(pn->pn_left, &expr) &&
>                statement(pn->pn_right, &stmt) &&
>-               (PN_TYPE(pn) == TOK_WITH)
>+               (pn->isKind(TOK_WITH))

Overparenthesized.

>-    JSParseNode *argsAndBody = (PN_TYPE(pn->pn_body) == TOK_UPVARS)
>+    JSParseNode *argsAndBody = (pn->pn_body->isKind(TOK_UPVARS))
>                                ? pn->pn_body->pn_tree
>                                : pn->pn_body;

Overparenthesized.

>diff --git a/js/src/jswin.h b/js/src/jswin.h
>--- a/js/src/jswin.h
>+++ b/js/src/jswin.h
>@@ -1,45 +1,46 @@
>-/* ***** BEGIN LICENSE BLOCK *****
>- * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>- *
>- * The contents of this file are subject to the Mozilla Public License Version
>- * 1.1 (the "License"); you may not use this file except in compliance with
>- * the License. You may obtain a copy of the License at
>- * http://www.mozilla.org/MPL/
>- *
>- * Software distributed under the License is distributed on an "AS IS" basis,
>- * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>- * for the specific language governing rights and limitations under the
>- * License.
>- *
>- * The Original Code is workarounds for the Win32 headers.
>- *
>- * The Initial Developer of the Original Code is
>- * the Mozilla Foundation.
>- * Portions created by the Initial Developer are Copyright (C) 2010
>- * the Initial Developer. All Rights Reserved.
>- *
>- * Contributor(s):
>- *
>- * Alternatively, the contents of this file may be used under the terms of
>- * either the GNU General Public License Version 2 or later (the "GPL"), or
>- * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>- * in which case the provisions of the GPL or the LGPL are applicable instead
>- * of those above. If you wish to allow use of your version of this file only
>- * under the terms of either the GPL or the LGPL, and not to allow others to
>- * use your version of this file under the terms of the MPL, indicate your
>- * decision by deleting the provisions above and replace them with the notice
>- * and other provisions required by the GPL or the LGPL. If you do not delete
>- * the provisions above, a recipient may use your version of this file under
>- * the terms of any one of the MPL, the GPL or the LGPL.
>- *
>- * ***** END LICENSE BLOCK ***** */
>-
>-/*
>- * This file is a wrapper around <windows.h> to prevent the mangling of
>- * various function names throughout the codebase.
>- */
>-#ifdef XP_WIN
>-# include <windows.h>
>-# undef GetProp
>-# undef SetProp
>-#endif
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is workarounds for the Win32 headers.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+/*
>+ * This file is a wrapper around <windows.h> to prevent the mangling of
>+ * various function names throughout the codebase.
>+ */
>+#ifdef XP_WIN
>+# include <windows.h>
>+# undef GetProp
>+# undef SetProp
>+# undef CONST
>+#endif

What happened here? Was this a line-endings change?

Overall, looks like a nice cleanup. r=me with nits addressed.

Dave

Comment 3

[Dave Herman [:dherman]]

Comment on attachment 544661 [details] [diff] [review]
The first shade of lipstick for Ms. Parser.

>-        if (pn->pn_type == TOK_FUNCTION) {
>-            JS_ASSERT(pn->pn_arity = PN_FUNC);

PS: I just re-read the description... nice catch.

>+        if (pn->isKind(TOK_FUNCTION)) {
>+            JS_ASSERT(pn->isArity(PN_FUNC));

Dave

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #2)

Thanks for catching the overparens! Will push with all nits addressed.

> >-                    pn2->pn_op = uint8(op);
> >+                    pn2->setOp(op);
> 
> I don't understand why the uint8 cast was here, or why you're eliminating it.

Yeah, doesn't make sense that it was there -- pn_op is only an 8 bit bitfield anyway.

> >-    // Grr, windows.h or something under it #defines CONST...
> >-#ifdef CONST
> >-# undef CONST
> >-#endif
> 
> Mmm, much less grumbly. I'm assuming this is tested and works in Windows
> (has windows.h changed?).

I moved it to jswin.h (along with the line endings changes), which is our typical windows.h wrapper include.

> What happened here? Was this a line-endings change?

Yeah, ran dos2unix on it because of all the explicit |^M|s.

Comment 5

[Nicholas Nethercote [inactive]]

Nice patch!

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> > >-                    pn2->pn_op = uint8(op);
> > >+                    pn2->setOp(op);
> > 
> > I don't understand why the uint8 cast was here, or why you're eliminating it.
> 
> Yeah, doesn't make sense that it was there -- pn_op is only an 8 bit
> bitfield anyway.

That's the reason. Some compilers (MSVC?) whine if you narrow an enum or wider integral type into a narrower unit of storage without a cast to prove you meant what you wrote.

/be

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #6)
> That's the reason. Some compilers (MSVC?) whine if you narrow an enum or
> wider integral type into a narrower unit of storage without a cast to prove
> you meant what you wrote.

But there are *lots* of other assignments from JSOp to that pn_op that have no such cast.

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4b5ceed32539