[webkit-reviews] review granted: [Bug 171265] test262: Completion values for control flow do not match the spec : [Attachment 311430] [PATCH] Proposed Fix v3

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 171265: test262: Completion values for control flow do not match the spec
https://bugs.webkit.org/show_bug.cgi?id=171265

Attachment 311430: [PATCH] Proposed Fix v3

https://bugs.webkit.org/attachment.cgi?id=311430&action=review




--- Comment #31 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 311430
  --> https://bugs.webkit.org/attachment.cgi?id=311430
[PATCH] Proposed Fix v3

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

r=me

> JSTests/mozilla/mozilla-tests.yaml:1035
> +  cmd: defaultRunMozillaTest :failDueToOutdatedOrBadTest, "../shell.js"

my favorite kind of failure.

> JSTests/stress/completion-value.js:20
> +// Statements that produce an (empty) completion value do not affect
results:

This test is great. Maybe it's so good we should give it to test262 if they
want it?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
> +    if (generator.shouldBeConcernedWithCompletionValue()) {

So this is the completion value of the "if" itself? Would it work just to put
this logic in SourceElements::emitBytecode?

Perhaps another way to phrase what I'm asking: If we have an
earlyBreakOrContinue(), are there places we actually get to skip caring about
the return value? Maybe it's worth consolidating this logic in
SourceElements::emitBytecode just for cleanliness?

I'm leaving it up to you since you understand the problem here much better than
I do.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3345
> +    if (generator.shouldBeConcernedWithCompletionValue() &&
m_tryBlock->hasEarlyBreakOrContinue())

ditto

> Source/JavaScriptCore/parser/Nodes.cpp:83
> +bool SourceElements::hasEarlyBreakOrContinue() const

nice!