900816 - Some TokenStream clean-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Patches coming shortly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace the TSF_OPERAND and TSF_KEYWORD_IS_NAME flags with a Modifier argument.

TokenStreamFlags is a mish-mash of things.  In particular, it mostly is a way
for TokenStream to record things that occurred during tokenizing.  However, the
TSF_OPERAND and TSF_KEYWORD_IS_NAME flags are exceptions -- they are instead
inputs from the Parser that temporarily modify how TokenStream operates.  They
are set via the Flagger class.

This patch removes those flags from TokenStreamFlags, and replaces them with
the TokenStream::Modifier enum.  getToken() et al now take an explicit Modifier
parameter (defaulting to |None|), instead of an implicit one via
TokenStream::flags. 

As well as being more concise and easier to read, it also reduces instruction
counts.  With clang, the Parsemark/Unreal improvements are 1.004x/1.010x.  With
GCC, they are 1.007x/1.019x.

Other changes of note:
- The patch folds MUST_MATCH_TOKEN_WITH_FLAGS into MUST_MATCH_TOKEN, because
  the former was never used.

- The |m.parser().statement(TSF_OPERAND)| call in ion/AsmJS.cpp was totally
  bogus -- statement() takes a bool arg, not a TokenStreamFlags.  I asked Luke
  and he said that no arg should be present (thus defaulting to |false|).

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use bitfields for TokenStream's flags.

Bitfields are so much more readable than direct bit manipulation.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move TokenStream's |isUnexpectedEOF| flag into Parser.

TokenStream::flags::isUnexpectedEOF is set by Parser and read by Parser, so it
should just live in Parser.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Always use |goto out| in non-error cases in getTokenInternal().

Most of the success paths in getTokenInternal() use |goto out|.  The exception
is the success paths in the switch under |c1kind == Other|, which use break.
This patch converts them to use |goto out| for consistency.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Rename some Token kinds.

We have the following token names:

    TOK_ADDASSIGN
    TOK_SUBASSIGN
    TOK_MULASSIGN
    TOK_DIVASSIGN
    TOK_MODASSIGN

And we also have:

    TOK_PLUS
    TOK_MINUS
    TOK_STAR (!)
    TOK_DIV
    TOK_MOD

And we also have the following ops:

    JSOP_ADD
    JSOP_SUB
    JSOP_MUL
    JSOP_DIV
    JSOP_MOD

This patch renames the following Token kinds:

    TOK_PLUS  --> TOK_ADD
    TOK_MINUS --> TOK_SUB
    TOK_STAR  --> TOK_MUL

Which makes everything consistent.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Use C++-style comments in TokenStream.{h,cpp}.

Because C-style comments make baby angels cry.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 784780 [details] [diff] [review]
(part 1) - Replace the TSF_OPERAND and TSF_KEYWORD_IS_NAME flags with a Modifier argument.

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

lovely

::: js/src/frontend/TokenStream.h
@@ +385,5 @@
> +// ignored unless |lookahead == 0| holds.  Due to constraints of the grammar,
> +// this turns out not to be a problem in practice. See the
> +// mozilla.dev.tech.js-engine.internals thread entitled 'Bug in the scanner?'
> +// for more details:
> +// https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.tech.js-engine.internals/2JLH5jRcr7E).

I'm not looking forward to the day Google switches off groups and all these links become useless.

@@ +478,5 @@
> +        None,           // Normal operation.
> +        Operand,        // Looking for an operand, not an operator.  In
> +                        //   practice, this means that when '/' is seen,
> +                        //   we look for a regexp instead of just returning
> +                        //   TOK_DIV.

the comment's internal indentation seems off

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 784782 [details] [diff] [review]
(part 2) - Use bitfields for TokenStream's flags.

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

indeed

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 784783 [details] [diff] [review]
(part 3) - Move TokenStream's |isUnexpectedEOF| flag into Parser.

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

yes

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 784784 [details] [diff] [review]
(part 4) - Always use |goto out| in non-error cases in getTokenInternal().

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

makes sense

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 784786 [details] [diff] [review]
(part 5) - Rename some Token kinds.

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

truly

::: js/src/frontend/TokenStream.h
@@ +129,2 @@
>      TOK_DIV,                       /* divide */
>      TOK_MOD,                       /* modulus */

As far as I'm concerned, you might just as well remove these comments. I'm not, however, really concerned, so up to you.

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 784787 [details] [diff] [review]
(part 6) - Use C++-style comments in TokenStream.{h,cpp}.

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

This is a surprisingly good change. I didn't think so when reading the description, but it really is great.

::: js/src/frontend/TokenStream.cpp
@@ +1211,5 @@
>      }
>  
> +    //
> +    // Look for a string.
> +    //

why the leading and following empty comment lines?

@@ +1311,5 @@
>      }
>  
> +    //
> +    // Skip over EOL chars, updating line state along the way.
> +    //

here, too

@@ +1402,5 @@
>      }
>  
> +    //
> +    // This handles everything else.
> +    //

and here

@@ +1508,5 @@
>  
>        case '/':
> +        //
> +        // Look for a single-line comment.
> +        //

here (also, having this be a single single-line comment would be nice)

@@ +1526,5 @@
>          }
>  
> +        //
> +        // Look for a multi-line comment.
> +        //

here, otoh, I'd personally leave the C-style comment. Because.

@@ +1548,5 @@
>          }
>  
> +        //
> +        // Look for a regexp.
> +        //

here

::: js/src/frontend/TokenStream.h
@@ +723,5 @@
>              return false;
>          }
>  
>          bool matchRawCharBackwards(jschar c) {
> +            JS_ASSERT(ptr);     // make sure haven't been poisoned

If you want, you could fix the comment here and twice below.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2be20e119af3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d72af267a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a09387c82fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe517199cd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/359fe5d08d54
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab85b5ed589

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2be20e119af3
https://hg.mozilla.org/mozilla-central/rev/c1d72af267a5
https://hg.mozilla.org/mozilla-central/rev/9a09387c82fc
https://hg.mozilla.org/mozilla-central/rev/9fe517199cd2
https://hg.mozilla.org/mozilla-central/rev/359fe5d08d54
https://hg.mozilla.org/mozilla-central/rev/4ab85b5ed589