Closed Bug 900816 Opened 7 years ago Closed 7 years ago

Some TokenStream clean-ups

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: [js:t])

Attachments

(6 files)

Patches coming shortly.
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|).
Attachment #784780 - Flags: review?(till)
Bitfields are so much more readable than direct bit manipulation.
Attachment #784782 - Flags: review?(till)
TokenStream::flags::isUnexpectedEOF is set by Parser and read by Parser, so it
should just live in Parser.
Attachment #784783 - Flags: review?(till)
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.
Attachment #784784 - Flags: review?(till)
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.
Attachment #784786 - Flags: review?(till)
Because C-style comments make baby angels cry.
Attachment #784787 - Flags: review?(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
Attachment #784780 - Flags: review?(till) → review+
Comment on attachment 784782 [details] [diff] [review]
(part 2) - Use bitfields for TokenStream's flags.

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

indeed
Attachment #784782 - Flags: review?(till) → review+
Comment on attachment 784783 [details] [diff] [review]
(part 3) - Move TokenStream's |isUnexpectedEOF| flag into Parser.

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

yes
Attachment #784783 - Flags: review?(till) → review+
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
Attachment #784784 - Flags: review?(till) → review+
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.
Attachment #784786 - Flags: review?(till) → review+
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.
Attachment #784787 - Flags: review?(till) → review+
You need to log in before you can comment on or make changes to this bug.