Closed Bug 1403556 Opened 2 years ago Closed 2 years ago

ParseNodeKind should be an enum class

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: Yoric, Assigned: rofael, Mentored)

Details

(Whiteboard: [c++][js:tech-debt])

Attachments

(2 files, 3 obsolete files)

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.
(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.
(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)
Sounds good. Feel free to ping me if you have any question.
Flags: needinfo?(dteller)
(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)
Yes, that should work.
Flags: needinfo?(dteller)
(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...
Yes, I was planning to do this in several successive bugs.
Priority: -- → P3
Whiteboard: [c++] → [c++][js:tech-debt]
Hey, Sourav, did you make any progress? If there is something blocking you, please feel free to ask me!
Flags: needinfo?(sourav.mukherjee619)
(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)
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)
I'd like to have this one, please.
Flags: needinfo?(dteller)
(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)
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).
(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.
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.
Attached patch 1403556.patch (obsolete) — Splinter Review
Compiled and ran successfully.
Attachment #8923509 - Flags: review?(jdemooij)
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+
Eventually PNK_SUPERCALL should be ParseNodeKind::SuperCall I think. Do you agree with these changes Waldo?
Flags: needinfo?(jwalden+bmo)
(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.
ParseNodeKind::SuperCall and InterCaps enum naming seems like where we've been heading generally, yeah -- fine with me.
Flags: needinfo?(jwalden+bmo)
Attached patch 1403556.patch (obsolete) — Splinter Review
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 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+
Attached patch 1403556.patchSplinter Review
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)
(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.
(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.
(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) :)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2f5ce41354
Changed ParseNodeKind from enum to enum class r=jandem
Marking this leave-open since there will be more patches :)
Mentor: dteller → jdemooij
Keywords: leave-open
Attachment #8937917 - Flags: review?(jdemooij) → review+
(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).
(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.
Attached patch 1403556-part2.patch (obsolete) — Splinter Review
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 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+
Attached patch 1403556-2.patchSplinter Review
Attachment #8938780 - Attachment is obsolete: true
Attachment #8939050 - Flags: review?(jdemooij)
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+
https://hg.mozilla.org/mozilla-central/rev/e5c3eb27d1b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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)
(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.