Closed
Bug 670049
Opened 13 years ago
Closed 13 years ago
Start adding accessors for pn_* members of JSParseNode
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: cdleary, Assigned: cdleary)
Details
Attachments
(1 file)
321.21 KB,
patch
|
dherman
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #544661 -
Flags: review?(dherman)
Comment 1•13 years ago
|
||
Newsletter, kplzthx.
Comment 2•13 years ago
|
||
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
Attachment #544661 -
Flags: review?(dherman) → review+
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
Nice patch!
Comment 6•13 years ago
|
||
(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
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4b5ceed32539
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•