Closed
Bug 1427710
Opened 6 years ago
Closed 6 years ago
Make TokenKind an enum class
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Updated•6 years ago
|
Mentor: jdemooij
Priority: -- → P3
Updated•6 years ago
|
Assignee: nobody → me
Assignee | ||
Comment 2•6 years ago
|
||
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)
Reporter | ||
Comment 3•6 years ago
|
||
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+
Assignee | ||
Comment 4•6 years ago
|
||
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
Reporter | ||
Comment 6•6 years ago
|
||
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+
Reporter | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5eb4b2bdb7cd
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
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+
Reporter | ||
Updated•6 years ago
|
Keywords: leave-open → checkin-needed
Comment 10•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7e7ac16af76a part 2 - Removed TOK_ prefixes. r=jandem
Keywords: checkin-needed
Comment 11•6 years ago
|
||
(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 )
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7e7ac16af76a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•