558002 - Convenience methods for TokenStream flags

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: TokenStream fixups.

Gets rid of the excess assert/tokenstream method/deassert code and hides the tokenstream flags except where necessary.

Comment 1

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

Attachment: TokenStream fixups.

Minor corrections.

Comment 2

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

Attachment: Token flag methods.

Half-indent the public keyword.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 438170 [details] [diff] [review]
Token flag methods.

Wow, was there was some fugliness with how this was previously done...

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp

> JSBool
> JSCompiler::argumentList(JSParseNode *listNode)
> {
>     JSBool matched;
> 
>-    tokenStream.flags |= TSF_OPERAND;
>-    matched = tokenStream.matchToken(TOK_RP);
>-    tokenStream.flags &= ~TSF_OPERAND;
>+    matched = tokenStream.matchToken(TOK_RP, TSF_OPERAND);

Unify declaration/initialization.


> JSBool
> JSCompiler::xmlElementContent(JSParseNode *pn)
> {
>-    TokenKind tt;
>-    JSParseNode *pn2;
>-    JSAtom *textAtom;
>-
>-    tokenStream.flags &= ~TSF_XMLTAGMODE;
>+    tokenStream.setXMLTagMode(false);
>     for (;;) {
>-        tokenStream.flags |= TSF_XMLTEXTMODE;
>-        tt = tokenStream.getToken();
>-        tokenStream.flags &= ~TSF_XMLTEXTMODE;
>+        TokenKind tt = tokenStream.getToken(TSF_XMLTEXTMODE);
>         XML_CHECK_FOR_ERROR_AND_EOF(tt, JS_FALSE);
> 
>         JS_ASSERT(tt == TOK_XMLSPACE || tt == TOK_XMLTEXT);
>-        textAtom = tokenStream.currentToken().t_atom;
>+        JSAtom *textAtom = tokenStream.currentToken().t_atom;
>+        JSParseNode *pn2;
>         if (textAtom) {
>             /* Non-zero-length XML text scanned. */
>             pn2 = xmlAtomNode();
>             if (!pn2)
>                 return JS_FALSE;
>             pn->pn_pos.end = pn2->pn_pos.end;
>             pn->append(pn2);
>         }

Don't share the same parsenode variable name with the use below -- make this one local to the if-block, then have a separate one for use later, since the two are entirely unrelated.


>@@ -7825,31 +7758,30 @@ JSCompiler::parseXMLText(JSObject *chain
>     /*
>      * Push a compiler frame if we have no frames, or if the top frame is a
>      * lightweight function activation, or if its scope chain doesn't match
>      * the one passed to us.
>      */
>     JSTreeContext xmltc(this);
>     xmltc.scopeChain = chain;
> 
>+    JSParseNode *pn;
>     /* Set XML-only mode to turn off special treatment of {expr} in XML. */
>-    tokenStream.flags |= TSF_OPERAND | TSF_XMLONLYMODE;
>-    TokenKind tt = tokenStream.getToken();
>-    tokenStream.flags &= ~TSF_OPERAND;
>-
>-    JSParseNode *pn;
>+    tokenStream.setXMLOnlyMode();
>+    TokenKind tt = tokenStream.getToken(TSF_OPERAND);
>+
>     if (tt != TOK_XMLSTAGO) {
>         ReportCompileErrorNumber(context, &tokenStream, NULL, JSREPORT_ERROR,
>                                  JSMSG_BAD_XML_MARKUP);
>         pn = NULL;
>     } else {
>         pn = xmlElementOrListRoot(allowList);
>     }

Why move the parsenode declaration like this?  It was better-placed where it was previously.


>diff --git a/js/src/jsscan.h b/js/src/jsscan.h

>     /* Mutators. */
>-    Token *mutableCurrentToken() { return &tokens[cursor]; }
>     bool reportCompileErrorNumberVA(JSParseNode *pn, uintN flags, uintN errorNumber, va_list ap);
>+    void mungCurrentToken(TokenKind newKind) { tokens[cursor].type = newKind; }
>+    void mungCurrentToken(JSOp newOp) { tokens[cursor].t_op = newOp; }
>+    void mungCurrentToken(TokenKind newKind, JSOp newOp) {
>+        mungCurrentToken(newKind);
>+        mungCurrentToken(newOp);
>+    }

Er, "munge"?


>+#define FLAG_METHODS(__methodName, __flagName)                                  \
>+    void set##__methodName(bool enabled = true) {                               \
>+        if (enabled)                                                            \
>+            flags |= (__flagName);                                              \
>+        else                                                                    \
>+            flags &= ~(__flagName);                                             \
>+    }                                                                           \
>+    bool is##__methodName() const { return flags & (__flagName); }
>+
>+    FLAG_METHODS(StrictMode, TSF_STRICT_MODE_CODE);
>+    FLAG_METHODS(XMLTagMode, TSF_XMLTAGMODE);
>+    FLAG_METHODS(XMLOnlyMode, TSF_XMLONLYMODE);
>+    FLAG_METHODS(UnexpectedEOF, TSF_UNEXPECTED_EOF);
>+
>+#undef FLAG_METHOD

I'd prefer if these weren't macro-ized, rather just have them each contain one-line bodies like |return setFlag(flagName);| and |return hasFlag(flagName);|.  It's not much more typing, and the debugging experience is much more pleasant


>+    class Flagger {

It seems preferable to have this class defined immediately above the uses of it.  Add another private section to the class/struct if necessary.


r=me with those changes

Comment 4

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

Attachment: Token flag methods.

We decided to change "mung" to "munge" for compatibility with the rest of the code base, despite the jargon file entry: http://catb.org/jargon/html/M/mung.html

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 438227 [details] [diff] [review]
Token flag methods.

>+#undef FLAG_METHOD

This is detritus now.

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/f02074309dcd

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f02074309dcd