1403556 - ParseNodeKind should be an enum class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

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

[sourav.mukherjee619]

(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

[sourav.mukherjee619]

(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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Sounds good. Feel free to ping me if you have any question.

Comment 4

[sourav.mukherjee619]

(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;

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Yes, that should work.

Comment 6

[Jan de Mooij [:jandem]]

(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...

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Yes, I was planning to do this in several successive bugs.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Hey, Sourav, did you make any progress? If there is something blocking you, please feel free to ask me!

Comment 9

[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 ?

Comment 10

[David Teller [:Yoric] - still alive but not very active]

No emergency, I just wanted to see if you needed help. Don't hesitate to ping me if you have any questions.

Comment 11

[Rofael Aleezada [:rofael]]

I'd like to have this one, please.

Comment 12

[Jan de Mooij [:jandem]]

(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 :)

Comment 13

[Rofael Aleezada [:rofael]]

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

[Jan de Mooij [:jandem]]

(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.

Comment 15

[Rofael Aleezada [:rofael]]

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.

Comment 16

[Rofael Aleezada [:rofael]]

Attachment: 1403556.patch

Compiled and ran successfully.

Comment 17

[Jan de Mooij [:jandem]]

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.)

Comment 18

[Jan de Mooij [:jandem]]

Eventually PNK_SUPERCALL should be ParseNodeKind::SuperCall I think. Do you agree with these changes Waldo?

Comment 19

[Rofael Aleezada [:rofael]]

(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

[Jeff Walden [:Waldo]]

ParseNodeKind::SuperCall and InterCaps enum naming seems like where we've been heading generally, yeah -- fine with me.

Comment 21

[Rofael Aleezada [:rofael]]

Attachment: 1403556.patch

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.

Comment 22

[Jan de Mooij [:jandem]]

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.

Comment 23

[Rofael Aleezada [:rofael]]

Attachment: 1403556.patch

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.

Comment 24

[Jan de Mooij [:jandem]]

(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.

Comment 25

[Rofael Aleezada [:rofael]]

(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

[Jan de Mooij [:jandem]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2f5ce41354
Changed ParseNodeKind from enum to enum class r=jandem

Comment 28

[Jan de Mooij [:jandem]]

Marking this leave-open since there will be more patches :)

Comment 29

[Jan de Mooij [:jandem]]

(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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cf2f5ce41354

Comment 31

[Rofael Aleezada [:rofael]]

(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.

Comment 32

[Rofael Aleezada [:rofael]]

Attachment: 1403556-part2.patch

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.

Comment 33

[Jan de Mooij [:jandem]]

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.

Comment 34

[Rofael Aleezada [:rofael]]

Attachment: 1403556-2.patch

Comment 35

[Jan de Mooij [:jandem]]

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? :)

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c3eb27d1b4
Remove PNK prefixes. r=jandem

Comment 37

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e5c3eb27d1b4

Comment 38

[Rofael Aleezada [:rofael]]

(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!

Comment 39

[Jan de Mooij [:jandem]]

(In reply to Rofael Aleezada [:rofael] from comment #38)
> Sure, just point me to the bug page!

I filed bug 1427710. Thanks!