Bug 124172 - Support unprefixed deconstructing assignment
Summary: Support unprefixed deconstructing assignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-11 16:50 PST by Oliver Hunt
Modified: 2013-11-12 12:53 PST (History)
1 user (show)

See Also:


Attachments
Patch (27.24 KB, patch)
2013-11-11 17:04 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (27.10 KB, patch)
2013-11-12 11:49 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (25.40 KB, patch)
2013-11-12 12:28 PST, Oliver Hunt
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-11-11 16:50:21 PST
Support unprefixed deconstructing assignment
Comment 1 Oliver Hunt 2013-11-11 17:04:39 PST
Created attachment 216624 [details]
Patch
Comment 2 Mark Lam 2013-11-11 18:57:03 PST
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.
Comment 3 Oliver Hunt 2013-11-12 11:49:17 PST
Created attachment 216698 [details]
Patch
Comment 4 Mark Lam 2013-11-12 12:03:59 PST
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?
Comment 5 Mark Lam 2013-11-12 12:05:37 PST
(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.
Comment 6 Oliver Hunt 2013-11-12 12:10:19 PST
(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.
Comment 7 Oliver Hunt 2013-11-12 12:28:36 PST
Created attachment 216703 [details]
Patch
Comment 8 Mark Lam 2013-11-12 12:37:50 PST
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.
Comment 9 Oliver Hunt 2013-11-12 12:53:35 PST
Committed r159139: <http://trac.webkit.org/changeset/159139>