Bug 124172

Created attachment 216624 [details] Patch

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.

Created attachment 216698 [details] Patch

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?

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

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

Created attachment 216703 [details] Patch

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.

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