Closed
Bug 1403556
Opened 7 years ago
Closed 6 years ago
ParseNodeKind should be an enum class
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: Yoric, Assigned: rofael, Mentored)
Details
(Whiteboard: [c++][js:tech-debt])
Attachments
(2 files, 3 obsolete files)
392.92 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
438.43 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Code would be a bit cleaner/safer if `ParseNodeKind`, as defined in ParseNode.h, was an `enum class` instead of an `enum`. To do this: 1. replace the definition of `ParseNodeKind` with an `enum class`; 2. see what it breaks; 3. fix it.
Comment 1•7 years ago
|
||
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #0) > Code would be a bit cleaner/safer if `ParseNodeKind`, as defined in > ParseNode.h, was an `enum class` instead of an `enum`. > > To do this: > 1. replace the definition of `ParseNodeKind` with an `enum class`; > 2. see what it breaks; > 3. fix it. I want to work on this one.
Comment 2•7 years ago
|
||
(In reply to sourav.mukherjee619 from comment #1) > (In reply to David Teller [:Yoric] (please use "needinfo") from comment #0) > > Code would be a bit cleaner/safer if `ParseNodeKind`, as defined in > > ParseNode.h, was an `enum class` instead of an `enum`. > > > > To do this: > > 1. replace the definition of `ParseNodeKind` with an `enum class`; > > 2. see what it breaks; > > 3. fix it. > > I want to work on this one.
Flags: needinfo?(dteller)
Reporter | ||
Comment 3•7 years ago
|
||
Sounds good. Feel free to ping me if you have any question.
Flags: needinfo?(dteller)
Comment 4•7 years ago
|
||
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #3) > Sounds good. Feel free to ping me if you have any question. Is this kind of change allright :- - return PNK_DELETENAME <= kind && kind <= PNK_DELETEEXPR; + return ParseNodeKind::PNK_DELETENAME <= kind && kind <= ParseNodeKind::PNK_DELETEEXPR;
Flags: needinfo?(dteller)
Comment 6•7 years ago
|
||
(In reply to sourav.mukherjee619 from comment #4) > Is this kind of change allright :- > > - return PNK_DELETENAME <= kind && kind <= PNK_DELETEEXPR; > + return ParseNodeKind::PNK_DELETENAME <= kind && kind <= > ParseNodeKind::PNK_DELETEEXPR; This makes sense as a first patch, but we should also remove the PNK_ prefix once it's an enum class. After that, it would be nice to rename ParseNodeKind::DELETENAME to ParseNodeKind::DeleteName etc...
Reporter | ||
Comment 7•7 years ago
|
||
Yes, I was planning to do this in several successive bugs.
Updated•7 years ago
|
Reporter | ||
Comment 8•7 years ago
|
||
Hey, Sourav, did you make any progress? If there is something blocking you, please feel free to ask me!
Flags: needinfo?(sourav.mukherjee619)
Comment 9•7 years ago
|
||
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #8) > Hey, Sourav, did you make any progress? If there is something blocking you, > please feel free to ask me! Sorry, I could not make any progress. Some important work came by. Can you assign it to someone else if the task is urgent ?
Flags: needinfo?(sourav.mukherjee619) → needinfo?(dteller)
Reporter | ||
Comment 10•7 years ago
|
||
No emergency, I just wanted to see if you needed help. Don't hesitate to ping me if you have any questions.
Flags: needinfo?(dteller)
Comment 12•7 years ago
|
||
(In reply to Rofael Aleezada [:rofael] from comment #11) > I'd like to have this one, please. Assigned it to you. Let us know if you have any questions :)
Assignee: nobody → me
Status: NEW → ASSIGNED
Flags: needinfo?(dteller)
Assignee | ||
Comment 13•7 years ago
|
||
Cool! I've done the obvious so far, replaced enum with enum class and added ParseNodeKind:: in front of all ~2200 instances of the enumerators being used outside the declaration. I've run into some new build errors with Parser.cpp. https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#7996 and https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#8039 tell me that operator+ and operator- need to be implemented, respectively. If I cast the enumerators as uint16_t, the build likes it. Should I implement the operators? (Casting seems too clunky for me).
Comment 14•7 years ago
|
||
(In reply to Rofael Aleezada [:rofael] from comment #13) > If I cast the enumerators as uint16_t, the build likes it. Should I > implement the operators? (Casting seems too clunky for me). I don't care too much, but one advantage of casting is that it's more explicit (no risk of someone accidentally using the operators). If it's just a few places I don't think it's too clunky.
Assignee | ||
Comment 15•7 years ago
|
||
Okay. I added the casts and it looks like it's compiling fine now. If it builds and runs without problem, I'll go ahead and put the patch up. Save for any minor changes, I won't be able to work on it again for the rest of the week, though.
Assignee | ||
Comment 16•7 years ago
|
||
Compiled and ran successfully.
Attachment #8923509 -
Flags: review?(jdemooij)
Comment 17•7 years ago
|
||
Comment on attachment 8923509 [details] [diff] [review] 1403556.patch Review of attachment 8923509 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. My comments below are just nits for manual style fixup - please check the patch for similar issues elsewhere. Also I assume you'll post a follow-up patch to remove the PNK_ prefix etc? :) ::: js/src/frontend/BytecodeEmitter.cpp @@ +3065,4 @@ > static JSOp > GetIncDecInfo(ParseNodeKind kind, bool* post) > { > + MOZ_ASSERT(kind == ParseNodeKind::PNK_POSTINCREMENT || kind == ParseNodeKind::PNK_PREINCREMENT || This no longer fits within 99 columns I think. Please add \n after each || @@ +3067,5 @@ > { > + MOZ_ASSERT(kind == ParseNodeKind::PNK_POSTINCREMENT || kind == ParseNodeKind::PNK_PREINCREMENT || > + kind == ParseNodeKind::PNK_POSTDECREMENT || kind == ParseNodeKind::PNK_PREDECREMENT); > + *post = kind == ParseNodeKind::PNK_POSTINCREMENT || kind == ParseNodeKind::PNK_POSTDECREMENT; > + return (kind == ParseNodeKind::PNK_POSTINCREMENT || kind == ParseNodeKind::PNK_PREINCREMENT) ? JSOP_ADD : JSOP_SUB; Here something like: return (kind == ... || kind == ...) ? JSOP_ADD : JSOP_SUB; @@ +5112,3 @@ > target = target->pn_left; > > + // No need to recur into ParseNodeKind::PNK_ARRAY and ParseNodeKind::PNK_OBJECT subpatterns here, since Please fix this comment to fit within 80 columns @@ +5115,3 @@ > // emitSetOrInitializeDestructuring does the recursion when setting or > // initializing value. Getting reference doesn't recur. > + if (target->isKind(ParseNodeKind::PNK_NAME) || target->isKind(ParseNodeKind::PNK_ARRAY) || target->isKind(ParseNodeKind::PNK_OBJECT)) \n after each || and add {} ::: js/src/frontend/FoldConstants.cpp @@ +50,4 @@ > // ParseNode contains any var statements. > // > // THIS IS NOT A GENERAL-PURPOSE FUNCTION. It is only written to work in the > +// specific context of deciding that |node|, as one arm of a ParseNodeKind::PNK_IF controlled 80 columns. @@ +175,4 @@ > return true; > } > > + // Legacy array and generator comprehensions use ParseNodeKind::PNK_IF to represent Fix this comment too. @@ +1529,4 @@ > FoldCall(JSContext* cx, ParseNode* node, Parser<FullParseHandler, char16_t>& parser, > bool inGenexpLambda) > { > + MOZ_ASSERT(node->isKind(ParseNodeKind::PNK_CALL) || node->isKind(ParseNodeKind::PNK_SUPERCALL) || \n after || @@ +1564,4 @@ > FoldForInOrOf(JSContext* cx, ParseNode* node, Parser<FullParseHandler, char16_t>& parser, > bool inGenexpLambda) > { > + MOZ_ASSERT(node->isKind(ParseNodeKind::PNK_FORIN) || node->isKind(ParseNodeKind::PNK_FOROF)); And here. ::: js/src/frontend/FullParseHandler.h @@ +70,4 @@ > // doesn't treat it as such. But we need to know when this happens to > // consider it a SyntaxError rather than an invalid-left-hand-side > // ReferenceError. > + return node->isInParens() && (node->isKind(ParseNodeKind::PNK_OBJECT) || node->isKind(ParseNodeKind::PNK_ARRAY)); \n after || @@ +194,4 @@ > } > > ParseNode* newTypeof(uint32_t begin, ParseNode* kid) { > + return newUnary(kid->isKind(ParseNodeKind::PNK_NAME) ? ParseNodeKind::PNK_TYPEOFNAME : ParseNodeKind::PNK_TYPEOFEXPR, begin, kid); Here it probably makes sense to add a ParseNodeKind local and then pass that to newUnary ::: js/src/frontend/NameFunctions.cpp @@ +42,4 @@ > * append '["name"]'. > * > * Note that we need the IsIdentifier check for atoms from both > + * ParseNodeKind::PNK_NAME nodes and ParseNodeKind::PNK_STRING nodes: given code like a["b c"], the Fix this comment. @@ +148,4 @@ > * the outer function is just a helper to create a scope for the > * returned function. Hence the name of the returned function should > * actually be 'foo'. This loop sees if the current node is a > + * ParseNodeKind::PNK_RETURN, and if there is a direct function call we skip to And this one. ::: js/src/frontend/ParseNode.h @@ +208,5 @@ > /* > * Label Variant Members > * ----- ------- ------- > * <Definitions> > + * ParseNodeKind::PNK_FUNCTION name pn_funbox: ptr to js::FunctionBox holding function I think we should remove the ParseNodeKind:: prefix in this comment - adding it just makes things messy. ::: js/src/frontend/Parser.cpp @@ +8532,4 @@ > if (!operand || !checkIncDecOperand(operand, operandOffset)) > return null(); > > + return handler.newUpdate((tt == TOK_INC) ? ParseNodeKind::PNK_PREINCREMENT : ParseNodeKind::PNK_PREDECREMENT, Maybe add a local ParseNodeKind variable here and then pass that instead. @@ +8585,4 @@ > tokenStream.consumeKnownToken(tt); > if (!checkIncDecOperand(expr, begin)) > return null(); > + return handler.newUpdate((tt == TOK_INC) ? ParseNodeKind::PNK_POSTINCREMENT : ParseNodeKind::PNK_POSTDECREMENT, And here. ::: js/src/frontend/Parser.h @@ +688,5 @@ > // They should be non-null only when Parser::forHeadStart parses a > // declaration at the start of a for-loop head. > // > + // In this case, on success |*forHeadKind| is ParseNodeKind::PNK_FORHEAD, ParseNodeKind::PNK_FORIN, or > + // ParseNodeKind::PNK_FOROF, corresponding to the three for-loop kinds. The precise value Fix this comment. ::: js/src/wasm/AsmJS.cpp @@ +6309,5 @@ > static bool > CheckComparison(FunctionValidator& f, ParseNode* comp, Type* type) > { > + MOZ_ASSERT(comp->isKind(ParseNodeKind::PNK_LT) || comp->isKind(ParseNodeKind::PNK_LE) || comp->isKind(ParseNodeKind::PNK_GT) || > + comp->isKind(ParseNodeKind::PNK_GE) || comp->isKind(ParseNodeKind::PNK_EQ) || comp->isKind(ParseNodeKind::PNK_NE)); Nit: \n after each || @@ +6388,4 @@ > int32_t identityElement; > bool onlyOnRight; > switch (bitwise->getKind()) { > + case ParseNodeKind::PNK_BITOR: identityElement = 0; onlyOnRight = false; *type = Type::Signed; break; Should probably \n after each ':' and ';' here. (This is exactly why I don't like this "stuff everything on one line" pattern.)
Attachment #8923509 -
Flags: review?(jdemooij) → feedback+
Comment 18•7 years ago
|
||
Eventually PNK_SUPERCALL should be ParseNodeKind::SuperCall I think. Do you agree with these changes Waldo?
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #17) Thanks for the feedback! Like I said, it'll probably take me at least a week to get to it.
Comment 20•7 years ago
|
||
ParseNodeKind::SuperCall and InterCaps enum naming seems like where we've been heading generally, yeah -- fine with me.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 21•7 years ago
|
||
I made all of the formatting changes. As for the local variable stuff, I only implemented it in the examples you explicitly gave to make sure I did it right.
Attachment #8923509 -
Attachment is obsolete: true
Attachment #8931507 -
Flags: review?(jdemooij)
Comment 22•7 years ago
|
||
Comment on attachment 8931507 [details] [diff] [review] 1403556.patch Review of attachment 8931507 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but this patch is based on a m-c revision of > 4 weeks ago so there's probably a lot of bitrot. If you can post an updated patch I can review quickly. ::: js/src/builtin/ReflectParse.cpp @@ +3526,4 @@ > for (ParseNode* arg = pnargs->pn_head; arg && arg != pnargs->last(); arg = arg->pn_next) { > ParseNode* pat; > ParseNode* defNode; > + if (arg->isKind(ParseNodeKind::PNK_NAME) || arg->isKind(ParseNodeKind::PNK_ARRAY) || arg->isKind(ParseNodeKind::PNK_OBJECT)) { if (arg->isKind(ParseNodeKind::PNK_NAME) || arg->isKind(ParseNodeKind::PNK_ARRAY) || arg->isKind(ParseNodeKind::PNK_OBJECT)) { @@ +3535,5 @@ > defNode = arg->pn_right; > } > > // Process the name or pattern. > + MOZ_ASSERT(pat->isKind(ParseNodeKind::PNK_NAME) || pat->isKind(ParseNodeKind::PNK_ARRAY) || pat->isKind(ParseNodeKind::PNK_OBJECT)); \n after each || ::: js/src/frontend/BytecodeEmitter.cpp @@ +4877,4 @@ > bool > BytecodeEmitter::emitSetThis(ParseNode* pn) > { > + // ParseNodeKind::PNK_SETTHIS is used to update |this| after a super() call in a derived Nit: make this fit in 80 columns (linebreak after "call" I think) ::: js/src/frontend/FullParseHandler.h @@ +581,4 @@ > } > > ParseNode* newComprehensionFor(uint32_t begin, ParseNode* forHead, ParseNode* body) { > + // A ParseNodeKind::PNK_COMPREHENSIONFOR node is binary: left is loop control, right I was going to comment on the line length, but we removed newComprehensionFor.
Attachment #8931507 -
Flags: review?(jdemooij) → feedback+
Assignee | ||
Comment 23•6 years ago
|
||
Now that I'm free of class work, time to finally finish this bug! I ended up restarting (My build was so old and there's been quite a few changes) and reimplementing all the formatting described in the previous comments. However, I wouldn't be surprised if I missed some due to all the changes.
Attachment #8931507 -
Attachment is obsolete: true
Attachment #8937917 -
Flags: review?(jdemooij)
Comment 24•6 years ago
|
||
(In reply to Rofael Aleezada [:rofael] from comment #23) > Now that I'm free of class work, time to finally finish this bug! \o/ This looks good. I have some style nits but I can fix these locally, so we can land this before it bitrots too much :) I'll do that today or tomorrow.
Assignee | ||
Comment 25•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #24) Sounds good! >Also I assume you'll post a follow-up patch to remove the PNK_ prefix etc? :) I'l start this once this patch lands.
Comment 26•6 years ago
|
||
(In reply to Rofael Aleezada [:rofael] from comment #25) > >Also I assume you'll post a follow-up patch to remove the PNK_ prefix etc? :) > > I'l start this once this patch lands. Here I would suggest converting one kind at a time, so ParseNodeKind::PNK_FORIN -> ParseNodeKind::ForIn. The nice thing is that we can do this incrementally with a separate patch for each kind (or a few kinds) :)
Comment 27•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2f5ce41354 Changed ParseNodeKind from enum to enum class r=jandem
Comment 28•6 years ago
|
||
Marking this leave-open since there will be more patches :)
Mentor: dteller → jdemooij
Keywords: leave-open
Updated•6 years ago
|
Attachment #8937917 -
Flags: review?(jdemooij) → review+
Comment 29•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #26) > Here I would suggest converting one kind at a time, so > ParseNodeKind::PNK_FORIN -> ParseNodeKind::ForIn. The nice thing is that we > can do this incrementally with a separate patch for each kind (or a few > kinds) :) Hm, this doesn't work because we have a macro to prepend PNK_ for each kind. We could first add the PNK_ prefix to each name in the FOR_EACH_PARSE_NODE_KIND list and them remove them incrementally, or we could just convert them all at once (should be safe since it's already an enum class).
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf2f5ce41354
Assignee | ||
Comment 31•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #29) > (In reply to Jan de Mooij [:jandem] from comment #26) > > Here I would suggest converting one kind at a time, so > > ParseNodeKind::PNK_FORIN -> ParseNodeKind::ForIn. The nice thing is that we > > can do this incrementally with a separate patch for each kind (or a few > > kinds) :) > > Hm, this doesn't work because we have a macro to prepend PNK_ for each kind. > We could first add the PNK_ prefix to each name in the > FOR_EACH_PARSE_NODE_KIND list and them remove them incrementally, or we > could just convert them all at once (should be safe since it's already an > enum class). I'll do it all in one patch, it would be easier for me.
Assignee | ||
Comment 32•6 years ago
|
||
And all the enumerators have been renamed! I used a script to do it, so there's probably spacing errors in some parts. Let me know if you'd like me to fix that or change any of the names.
Attachment #8938780 -
Flags: review?(jdemooij)
Comment 33•6 years ago
|
||
Comment on attachment 8938780 [details] [diff] [review] 1403556-part2.patch Review of attachment 8938780 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. Much nicer! Just some nits below. ::: js/src/frontend/ParseNode.h @@ +43,4 @@ > class ObjectBox; > > #define FOR_EACH_PARSE_NODE_KIND(F) \ > + F(NOp) \ Nit: Nop would be more consistent with code elsewhere (see MNop in jit/MIR.h for example). @@ +144,5 @@ > /* \ > * Binary operators. \ > * These must be in the same order as TOK_OR and friends in TokenStream.h. \ > */ \ > + F(PipeLine) \ Nit: Pipeline @@ +155,1 @@ > F(EQ) \ Nit: I'd rename to Eq, Ne, Lt, Gt, StrictEq, StrictNe, etc. (We could also go with Equal, NotEqual, LessThan, GreaterThanOrEqual, StrictEqual, etc, but that's better done as a followup.) @@ +162,5 @@ > + F(InstanceOf) \ > + F(In) \ > + F(LSh) \ > + F(RSh) \ > + F(URSh) \ Nit: Ion has MLsh/MRsh/MUrsh classes, so Lsh/Rsh/Ursh would be more consistent @@ +180,5 @@ > + F(BitXorAssign) \ > + F(BitAndAssign) \ > + F(LShAssign) \ > + F(RShAssign) \ > + F(URShAssign) \ Same here.
Attachment #8938780 -
Flags: review?(jdemooij) → feedback+
Assignee | ||
Comment 34•6 years ago
|
||
Attachment #8938780 -
Attachment is obsolete: true
Attachment #8939050 -
Flags: review?(jdemooij)
Comment 35•6 years ago
|
||
Comment on attachment 8939050 [details] [diff] [review] 1403556-2.patch Review of attachment 8939050 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks! Maybe you'd be interested in converting TokenKind next? :)
Attachment #8939050 -
Flags: review?(jdemooij) → review+
Updated•6 years ago
|
Keywords: leave-open → checkin-needed
Comment 36•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c3eb27d1b4 Remove PNK prefixes. r=jandem
Keywords: checkin-needed
Comment 37•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5c3eb27d1b4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #35) > Comment on attachment 8939050 [details] [diff] [review] > 1403556-2.patch > > Review of attachment 8939050 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great, thanks! > > Maybe you'd be interested in converting TokenKind next? :) Sure, just point me to the bug page!
Flags: needinfo?(jdemooij)
Comment 39•6 years ago
|
||
(In reply to Rofael Aleezada [:rofael] from comment #38) > Sure, just point me to the bug page! I filed bug 1427710. Thanks!
Flags: needinfo?(jdemooij)
You need to log in
before you can comment on or make changes to this bug.
Description
•