1477157 - Store the info about the existence of the default case into the switch ParseNode.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

derived from bug 1456006.

We already know the existence of the default case while parsing switch statement (both normal JS and BinJS).
We should store it into switch ParseNode so that we don't have to iterate over cases in order to check if there's default case.

we could use either:
  * the third field in BinaryNode
  * different ParseNodeKind, maybe SwitchWithDefault

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Store the info about the existence of the default case into the switch ParseNode.

Added Switch class which inherits from ParseNode. it uses the 3rd field of binary to store a bool value which indicates if there's a default case.
and changed consumers of the node to use Switch class, and changed the loop for generating table in emitSwitch to break immediately when it becomes invalid.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8994095 [details] [diff] [review]
Store the info about the existence of the default case into the switch ParseNode.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2387,5 @@
>              ParseNode* caseValue = caseNode->caseExpression();
>  
>              if (caseValue->getKind() != ParseNodeKind::Number) {
>                  tableGen.setInvalid();
> +                break;

Hmm, looking harder at this, there's no reason we can't track this as we parse cases, right?  Maybe worth another followup to check this as we go rather than have this second loop.

::: js/src/frontend/ParseNode.h
@@ +555,5 @@
>              ParseNode*  right;
>              union {
>                  unsigned iflags;        /* JSITER_* flags for ParseNodeKind::For node */
>                  bool isStatic;          /* only for ParseNodeKind::ClassMethod */
> +                bool hasDefault;        /* only for ParseNodeKind::Switch */

Could you make this field private and friend the Switch class that enables access to it, without much wider change?  Same thing as we do for ParseNode::pn_u::loopControl.

@@ +1271,5 @@
>          return pn_u.binary.isStatic;
>      }
>  };
>  
> +struct Switch : public BinaryNode {

A part of me would prefer if this were SwitchStatement -- Switch feels a little too short and generic.  Not super-strongly, but it is how I am inclined.

@@ +1273,5 @@
>  };
>  
> +struct Switch : public BinaryNode {
> +    /*
> +     * Method defintions often keep a name and function body that overlap,

definitions

Although, this looks like copypasta from the old spot that should just be removed, no?

@@ +1281,5 @@
> +      : BinaryNode(ParseNodeKind::Switch, JSOP_NOP,
> +                   TokenPos(begin, lexicalForCaseList->pn_pos.end),
> +                   discriminant, lexicalForCaseList)
> +    {
> +        pn_u.binary.hasDefault = hasDefault;

Can we quickly run through |lexicalForCaseList| and assert that |hasDefault| has the right value?

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] from comment #2)
> ::: js/src/frontend/ParseNode.h
> @@ +555,5 @@
> >              ParseNode*  right;
> >              union {
> >                  unsigned iflags;        /* JSITER_* flags for ParseNodeKind::For node */
> >                  bool isStatic;          /* only for ParseNodeKind::ClassMethod */
> > +                bool hasDefault;        /* only for ParseNodeKind::Switch */
> 
> Could you make this field private and friend the Switch class that enables
> access to it, without much wider change?  Same thing as we do for
> ParseNode::pn_u::loopControl.

it's not possible because it's anonymous union which cannot have private member, according to the following error message:

> .../mozilla-unified/js/src/frontend/ParseNode.h:562:24: error: anonymous union can only contain
>       non-static data members
>                 friend struct SwitchStatement;
>                        ^
> .../mozilla-unified/js/src/frontend/ParseNode.h:563:22: error: anonymous union cannot contain a
>       private data member
>                 bool hasDefault;        /* only for ParseNodeKind::Switch */
>                      ^

I'll leave it to other bug, in order to handle other cases at the same time, by changing them non-anonymous and wrapping all accesses to these inner unions/structs by dedicated class'es accessors.

Comment 4

[Tooru Fujisawa [:arai]]

looks like it's already filed as bug 1476805.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3efa24f1f0ce19ba688e4fecd43aba07a0e07710
Bug 1477157 - Store the info about the existence of the default case into the switch ParseNode. r=jwalden

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3efa24f1f0ce