Closed Bug 1427710 Opened 6 years ago Closed 6 years ago

Make TokenKind an enum class

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: rofael, Mentored)

References

Details

Attachments

(2 files, 1 obsolete file)

Similar to bug 1403556. Filing this bug for :rofael.
I guess I'll do the same thing as last time. One patch t0 change it to an enum class, and a second patch to remove the prefixes.
Mentor: jdemooij
Priority: -- → P3
Assignee: nobody → me
Attached patch 1427710-part1.patch (obsolete) — Splinter Review
Here's part 1! Let me know if there's any formatting issues I missed, or if the parts that I casted to int should be something else.
Attachment #8941692 - Flags: review?(jdemooij)
Comment on attachment 8941692 [details] [diff] [review]
1427710-part1.patch

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

Looks good! Some minor comments but should be good to go with that fixed.

::: js/src/frontend/Parser.cpp
@@ +2869,3 @@
>      if (!tokenStream.peekTokenSameLine(&tt, TokenStream::Operand))
>          return false;
> +    if (tt != TokenKind::TOK_EOF && tt != TokenKind::TOK_EOL && tt != TokenKind::TOK_SEMI && tt != TokenKind::TOK_RC) {

Format this like:

if (tt != TokenKind::TOK_EOF &&
    tt != TokenKind::TOK_EOL &&
    ...)
{

@@ +3075,4 @@
>                  if (!tokenStream.getToken(&tt))
>                      return false;
>  
> +                if (!TokenKindIsPossibleIdentifier(tt) && tt != TokenKind::TOK_LB && tt != TokenKind::TOK_LC) {

And similar here.

@@ +7831,4 @@
>  BinaryOpTokenKindToParseNodeKind(TokenKind tok)
>  {
>      MOZ_ASSERT(TokenKindIsBinaryOp(tok));
> +    return ParseNodeKind(size_t(ParseNodeKind::BinOpFirst) + ((int) tok - (int) TokenKind::TOK_BINOP_FIRST));

(size_t(tok) - size_t(TokenKind::TOK_BINOP_FIRST))

::: js/src/frontend/Parser.h
@@ +1232,4 @@
>      PropertyName* bindingIdentifier(YieldHandling yieldHandling);
>  
>      bool checkLabelOrIdentifierReference(PropertyName* ident, uint32_t offset,
> +                                         YieldHandling yieldHandling, TokenKind hint = TokenKind::TOK_LIMIT);

\n after yieldHandling,

::: js/src/frontend/TokenKind.h
@@ +302,5 @@
>  
>  inline MOZ_MUST_USE bool
>  TokenKindIsReservedWordLiteral(TokenKind tt)
>  {
> +    return TokenKind::TOK_RESERVED_WORD_LITERAL_FIRST <= tt && tt <= TokenKind::TOK_RESERVED_WORD_LITERAL_LAST;

These seem okay to keep for now since the next patch will shrink these names.

::: js/src/frontend/TokenStream.cpp
@@ +426,4 @@
>  
>      // See Parser::assignExpr() for an explanation of isExprEnding[].
>      PodArrayZero(isExprEnding);
> +    isExprEnding[(int) TokenKind::TOK_COMMA] = 1;

Nit: I'd prefer a C++-style size_t cast here and elsewhere:

isExprEnding[size_t(TokenKind::TOK_COMMA)] = 1;

@@ +1252,4 @@
>  static bool
>  IsTokenSane(Token* tp)
>  {
> +    // Nb: TokenKind::TOK_EOL should never be used in an actual Token;  it should only be

Nit: wrap comment to 80 chars.

@@ +1356,4 @@
>      // of the tokens seen in practice.
>      //
>      // We represent the 'OneChar' kind with any positive value less than
> +    // TokenKind::TOK_LIMIT.  This representation lets us associate each one-char token

And here.

@@ +1404,1 @@
>  /*  10+ */     EOL,   Space,   Space,     EOL, _______, _______, _______, _______, _______, _______,

Thanks for fixing the formatting of these tables!
Attachment #8941692 - Flags: review?(jdemooij) → feedback+
I think I got all of them
Attachment #8941692 - Attachment is obsolete: true
Attachment #8942025 - Flags: review?(jdemooij)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb4b2bdb7cd
part 1 - Make TokenKind an enum class. r=jandem
Comment on attachment 8942025 [details] [diff] [review]
1427710-part1.patch

Looks good to me! I pushed this to avoid bitrot.
Attachment #8942025 - Flags: review?(jdemooij) → review+
Keywords: leave-open
Comment on attachment 8944209 [details]
Bug 1427710 part 2 - Removed TOK_ prefixes.

https://reviewboard.mozilla.org/r/214498/#review220196

Looks great, thanks!

At some point we should probably rename TokenKind::Lp/Lc/Lb to TokenKind::LeftParen/LeftCurly/LeftBracket, but that's not for this bug.
Attachment #8944209 - Flags: review?(jdemooij) → review+
Blocks: 1432135
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7e7ac16af76a
part 2 - Removed TOK_ prefixes. r=jandem
Keywords: checkin-needed
(In reply to Jan de Mooij [:jandem] from comment #9)
> At some point we should probably rename TokenKind::Lp/Lc/Lb to
> TokenKind::LeftParen/LeftCurly/LeftBracket, but that's not for this bug.

Not TokenKind::{Open,Close}Stache?  http://fold.sigusr2.net/2010/03/mustache.html  :-D  (h/t https://twitter.com/dolske/status/10287903975 )
https://hg.mozilla.org/mozilla-central/rev/7e7ac16af76a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: