Remove some ParseNode morphing across types

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Let's just say morphing is kind of crazy.

ParseNode::become() is barely used; it can go.
(Assignee)

Comment 1

5 years ago
Created attachment 699856 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #699856 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 699872 [details] [diff] [review]
part 2, v1

Separate, because I'm not sure about it. I don't see why this hack would be necessary. I can't come up with code that compiles, and then round-trips incorrectly due to the EmptyStatement being removed. This, for example:

    a: for (;;) {
        a: ;
        for (;;) { break a; }
    }

would round-trip incorrectly, except that it's not legal JS. The duplicate label triggers a SyntaxError.
Attachment #699872 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 700619 [details] [diff] [review]
part 3, v1

What the heck, have another.
Attachment #700619 - Flags: review?(jwalden+bmo)

Comment 4

5 years ago
Comment on attachment 699856 [details] [diff] [review]
v1

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

::: js/src/frontend/FoldConstants.cpp
@@ +196,5 @@
> +    //
> +    // pnp points to a ParseNode pointer. This must be the only pointer that
> +    // points to the parse node being replaced. The replacement, *pn, is
> +    // unchanged except for its pn_next pointer; updating that is necessary if
> +    // *pn's new parent is a list node.

Erm, shouldn't this be outside the method, as a whole-method comment?

@@ +435,5 @@
>  
>          /* Don't fold a parenthesized call expression. See bug 537673. */
> +        ParseNode **listp = &pn->pn_head;
> +        if ((pn->isKind(PNK_CALL) || pn->isKind(PNK_NEW)) && (*listp)->isInParens())
> +            listp = &(*listp)->pn_next;

This has to be pretty close to the worst of the Augean stables here.  Right?  I hope?  Pretty please?

Okay, probably not.

::: js/src/frontend/FoldConstants.h
@@ +18,1 @@
>                bool inCond = false);

This method needs a comment to explain why we're passing ParseNode**, and to demonstrate how to use it.  It's understandable if you consult all the uses, but that really shouldn't be necessary!
Attachment #699856 - Flags: review?(jwalden+bmo) → review+

Comment 5

5 years ago
Comment on attachment 699872 [details] [diff] [review]
part 2, v1

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

...I have no idea at all about this.  Throw a fuzzer at it, have it find any problems that might exist with this.  rs=me
Attachment #699872 - Flags: review?(jwalden+bmo) → review+

Comment 6

5 years ago
Comment on attachment 700619 [details] [diff] [review]
part 3, v1

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

::: js/src/frontend/ParseNode.cpp
@@ +263,5 @@
>      return parser->new_<ParseNode>(kind, JSOP_NOP, arity, tok.pos);
>  }
>  
>  ParseNode *
> +ParseNode::append(ParseNodeKind kind, JSOp op, ParseNode *left, ParseNode *right, Parser *parser)

Can't this return ListNode* now?

@@ +278,3 @@
>          ParseNode *pn1 = left->pn_left, *pn2 = left->pn_right;
> +        list = parser->new_<ListNode>(kind, op);
> +        list->pn_pos = left->pn_pos;

It would be more consistent with the other ParseNode subclass constructors to set the position as part of the constructor.  That said, I'm not 100% certain what it would make sense to pass in here, exactly.  Should ListNode perhaps take in a single node to use as its sole child, after initial construction, and derive begin/end from that?  That seems best to me, but maybe you have other thoughts on this.

::: js/src/frontend/ParseNode.h
@@ +1021,5 @@
>  #endif
>  };
>  
>  struct ListNode : public ParseNode {
> +    ListNode(ParseNodeKind kind, JSOp op)

struct ListNode : public ParseNode
{

@@ +1022,5 @@
>  };
>  
>  struct ListNode : public ParseNode {
> +    ListNode(ParseNodeKind kind, JSOp op)
> +        : ParseNode(kind, op, PN_LIST)

Should be indented two spaces fewer.
Attachment #700619 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Erm, shouldn't this be outside the method, as a whole-method comment?

Duh. Yes, of course. Done.

No comment on the Augean stables.

> ::: js/src/frontend/FoldConstants.h
> @@ +18,1 @@
> >                bool inCond = false);
> 
> This method needs a comment to explain why we're passing ParseNode**, and to
> demonstrate how to use it.  It's understandable if you consult all the uses,
> but that really shouldn't be necessary!

OK, done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> >  ParseNode *
> > +ParseNode::append(ParseNodeKind kind, JSOp op, ParseNode *left, ParseNode *right, Parser *parser)
> 
> Can't this return ListNode* now?

Yes; I'll change the type in a subsequent patch.

> [...] Should ListNode
> perhaps take in a single node to use as its sole child, after initial
> construction, and derive begin/end from that?  That seems best to me, but
> maybe you have other thoughts on this.

All right. Changed.
(Assignee)

Comment 8

5 years ago
Created attachment 702596 [details] [diff] [review]
part 1, v2

carrying forward Waldo's r+.
Attachment #699856 - Attachment is obsolete: true
Attachment #702596 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 702597 [details] [diff] [review]
part 2, v2
Attachment #699872 - Attachment is obsolete: true
Attachment #702597 - Flags: review+
(Assignee)

Comment 10

5 years ago
Created attachment 702599 [details] [diff] [review]
part 3, v2

Fuzzers, please give this stack of patches a spin.
Attachment #702599 - Flags: review+
Attachment #702599 - Flags: feedback?(gary)
Attachment #702599 - Flags: feedback?(choller)
(Assignee)

Updated

5 years ago
Attachment #700619 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 702612 [details] [diff] [review]
consolidated patch
Attachment #702612 - Flags: feedback?(gary)
Attachment #702612 - Flags: feedback?(choller)
(Assignee)

Updated

5 years ago
Attachment #702599 - Flags: feedback?(gary)
Attachment #702599 - Flags: feedback?(choller)
Comment on attachment 702612 [details] [diff] [review]
consolidated patch

I didn't find anything particularly wrong here after a night of fuzzing - tentatively marking feedback+.
Attachment #702612 - Attachment description: consoliated patch → consolidated patch
Attachment #702612 - Flags: feedback?(gary) → feedback+
Comment on attachment 702612 [details] [diff] [review]
consolidated patch

Same here :)
Attachment #702612 - Flags: feedback?(choller) → feedback+
You need to log in before you can comment on or make changes to this bug.