Bug 124172 – Support unprefixed deconstructing assignment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Support unprefixed deconstructing assignment

[Oliver Hunt]

Created attachment 216624 [details]
Patch

[Mark Lam]

Comment on attachment 216624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216624&action=review

Can you please address the comments below?

> Source/JavaScriptCore/ChangeLog:15
> +        We're also able to predicate our attempt to parse on the existence of
> +        '[' or '{' as they not as common as other constructs. 

typo: they *are* not

> Source/JavaScriptCore/parser/Parser.cpp:430
> +    SavePoint start = createSavePoint();

Isn't this redundant?  The 2 caller sites that uses tryParseDeconstructionPatternExpression() also save a SavePoint.

> LayoutTests/js/script-tests/parser-syntax-check.js:391
> +valid("for ({of} in of){}")

The expected results said that this test FAILed.  This suggests that either this test should expect the statement to be "invalid" or the FAIL results are indicating there's a bug.  Can you please clarify which it's supposed to be?

> LayoutTests/js/script-tests/parser-syntax-check.js:430
> +valid("var {x: 1}=1")
> +valid("[x]=1)")

Ditto with FAILed results.

[Oliver Hunt]

Created attachment 216698 [details]
Patch

[Mark Lam]

Comment on attachment 216698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216698&action=review

> Source/JavaScriptCore/parser/Parser.cpp:431
> +template <class TreeBuilder> TreeDeconstructionPattern Parser<LexerType>::tryParseDeconstructionPatternExpression(TreeBuilder& context)
> +{
> +    return parseDeconstructionPattern<DeconstructToExpressions>(context);
> +}

Since this function no longer does anything extra anymore, it might make more sense to call parseDeconstructionPattern<DeconstructToExpressions>(context) from the 2 caller sites.

> LayoutTests/js/parser-syntax-check-expected.txt:621
> +FAIL Valid:   "function f() { for ({of} in of){} }" should NOT throw 

This one is still failing.  Is this appropriate?

[Mark Lam]

(In reply to comment #4)
> > Source/JavaScriptCore/parser/Parser.cpp:431
> > +template <class TreeBuilder> TreeDeconstructionPattern Parser<LexerType>::tryParseDeconstructionPatternExpression(TreeBuilder& context)
> > +{
> > +    return parseDeconstructionPattern<DeconstructToExpressions>(context);
> > +}
> 
> Since this function no longer does anything extra anymore, it might make more sense to call parseDeconstructionPattern<DeconstructToExpressions>(context) from the 2 caller sites.

I meant ... "to call parseDeconstructionPattern<DeconstructToExpressions>(context) *directly* from the 2 caller sites" and remove tryParseDeconstructionPatternExpression() altogether.

[Oliver Hunt]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/JavaScriptCore/parser/Parser.cpp:431
> > > +template <class TreeBuilder> TreeDeconstructionPattern Parser<LexerType>::tryParseDeconstructionPatternExpression(TreeBuilder& context)
> > > +{
> > > +    return parseDeconstructionPattern<DeconstructToExpressions>(context);
> > > +}
> > 
> > Since this function no longer does anything extra anymore, it might make more sense to call parseDeconstructionPattern<DeconstructToExpressions>(context) from the 2 caller sites.
> 
> I meant ... "to call parseDeconstructionPattern<DeconstructToExpressions>(context) *directly* from the 2 caller sites" and remove tryParseDeconstructionPatternExpression() altogether.

We bludgeon this with the NEVER_INLINE flag to prevent in adding stack usage that we'd like to avoid.

[Oliver Hunt]

Created attachment 216703 [details]
Patch

[Mark Lam]

Comment on attachment 216703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216703&action=review

r=me

> LayoutTests/js/parser-syntax-check-expected.txt:621
> +PASS Valid:   "for ({of} in of){} /**/"
> +PASS Valid:   "function f() { for ({of} in of){} /**/ }"

Remove the /**/ comment.

> LayoutTests/js/script-tests/parser-syntax-check.js:391
> +valid("for ({of} in of){} /**/")

Remove the /**/ comment.

[Oliver Hunt]

Committed r159139: <http://trac.webkit.org/changeset/159139>