1053240 - Add a higher-order macro for TokenKind

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140811180644

Steps to reproduce:

(Separated from bug 1041426)

Add a higher-order macro for TokenKind, like FOR_EACH_PARSE_NODE_KIND for ParseNodeKind.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Add a higher-order macro for TokenKind.

Added FOR_EACH_TOKEN_KIND macro, and use it in TokenKind enum and TokenKindToString function.

Second argument of F macro is a description of the token, which will be used in bug 1041426,
it's not used anywhere now, but could be alternative for old comments.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=ed6d55459ef7

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8473644 [details] [diff] [review]
Add a higher-order macro for TokenKind.

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

Good enough to start.  It'd be nice to further flesh out comments on these, maybe, but we should land this so it's in the tree, then deal with any polishing we care about.

::: js/src/frontend/TokenStream.cpp
@@ +1817,5 @@
>      switch (tt) {
> +      case TOK_ERROR:   return "TOK_ERROR";
> +#define EMIT_CASE(name,desc) \
> +      case TOK_##name:  return "TOK_" #name;
> +#define EMIT_NONE(name,value)

Space after commas.

::: js/src/frontend/TokenStream.h
@@ +26,5 @@
>  struct KeywordInfo;
>  
>  namespace js {
>  namespace frontend {
>  

Move the TokenKind definition macro, and the definition of TokenKind, into a new frontend/TokenKind.h file, then include it in this header.  This separates the TokenKind mechanics from the TokenStream mechanics, so that someone looking to focus just on how the stream works can do so, without having to skim past the kind details.

@@ +28,5 @@
>  namespace js {
>  namespace frontend {
>  
> +// Note that this macro does not contain ERROR and LIMIT.
> +#define FOR_EACH_TOKEN_KIND(F,RANGE) \

#define FOR_EACH_TOKEN_KIND(macro, range) \

"macro" instead of "f" to indicate the meaning/use, lowercase because shouty is less readable.

This needs a full doc comment by it explaining how to use it, giving a demo of how to use it, and explaining the meanings of the arguments passed to it.  Not because the first argument's so bad, mostly because the second argument is inscrutable.  :-)

@@ +101,5 @@
> +     * Binary operators tokens, TOK_OR thru TOK_MOD. These must be in the same \
> +     * order as F(OR) and friends in FOR_EACH_PARSE_NODE_KIND in ParseNode.h. \
> +     */ \
> +    F(OR,           "'||'")   /* logical or */ \
> +    RANGE(BINOP_FIRST, OR) \

...that's what range is for.  Blurgh.

@@ +170,5 @@
>  enum TokenKind {
> +    TOK_ERROR = 0,
> +#define EMIT_ENUM(name,desc) TOK_##name,
> +#define EMIT_ENUM_RANGE(name,value) TOK_##name = TOK_##value,
> +    FOR_EACH_TOKEN_KIND(EMIT_ENUM,EMIT_ENUM_RANGE)

Space after each comma (except in TOK_##value, of course).

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add a higher-order macro for TokenKind.

Thank you for reviewing!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> ::: js/src/frontend/TokenStream.h
> @@ +26,5 @@
> >  struct KeywordInfo;
> >  
> >  namespace js {
> >  namespace frontend {
> >  
> 
> Move the TokenKind definition macro, and the definition of TokenKind, into a
> new frontend/TokenKind.h file, then include it in this header.  This
> separates the TokenKind mechanics from the TokenStream mechanics, so that
> someone looking to focus just on how the stream works can do so, without
> having to skim past the kind details.

Moved TokenKind definitions and range-testing functions.

> @@ +28,5 @@
> >  namespace js {
> >  namespace frontend {
> >  
> > +// Note that this macro does not contain ERROR and LIMIT.
> > +#define FOR_EACH_TOKEN_KIND(F,RANGE) \
> 
> #define FOR_EACH_TOKEN_KIND(macro, range) \
> 
> "macro" instead of "f" to indicate the meaning/use, lowercase because shouty
> is less readable.
> 
> This needs a full doc comment by it explaining how to use it, giving a demo
> of how to use it, and explaining the meanings of the arguments passed to it.
> Not because the first argument's so bad, mostly because the second argument
> is inscrutable.  :-)

In that case, how about renaming old FOR_EACH_TOKEN_KIND to FOR_EACH_TOKEN_KIND_WITH_RANGE, and add new FOR_EACH_TOKEN_KIND without range.
Currently range data is used only in enum. and only first argument is used in other places (TokenKindToString, and TokenKindToDesc in bug 1041426).

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=8da9be74b28d

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8477808 [details] [diff] [review]
Add a higher-order macro for TokenKind.

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

I'll package this up to push probably later today, as these changes are super-minor.  (If I can get a current patch in shape, such that rebasing through it is feasible.)  Thanks!

::: js/src/frontend/TokenKind.h
@@ +7,5 @@
> +#ifndef frontend_TokenKind_h
> +#define frontend_TokenKind_h
> +
> +namespace js {
> +namespace frontend {

This is a very nitpicky thing, but I'd like to keep the macro outside the namespace -- because of course macros are defined irrespective of the namespace that "contains" them.  So move this just above the enum TokenKind definition.

::: js/src/frontend/TokenStream.cpp
@@ +1817,5 @@
>      switch (tt) {
> +      case TOK_ERROR:   return "TOK_ERROR";
> +#define EMIT_CASE(name, desc) \
> +      case TOK_##name:  return "TOK_" #name;
> +FOR_EACH_TOKEN_KIND(EMIT_CASE)

Minor preference for

    switch (tt) {
#define EMIT_CASE(name, desc) case TOK_##name: return "TOK_" #name;
      FOR_EACH_TOKEN_KIND(EMIT_CASE)
#undef EMIT_CASE
      case TOK_ERROR: return "TOK_ERROR";
      case TOK_LIMIT: break;
    }

so that the macro call is at the same level of indentation as the thing it would expand to.

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63c65da22baf

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63c65da22baf