Bug 217879 – test262: test/language/expressions/conditional/in-branch-1.js

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Cohen]

The in-branch-1 test262 test is failing, which checks that the `in` keyword is allowed in the LHS of a ternary (between `?` and `:`) even if that ternary is one of the expressions in a `for` statement.

[Mark Cohen]

Created attachment 411692 [details]
Patch

[Mark Cohen]

I ran test262 before and after and I can confirm that this patch only affects in-branch-1. I also ran run-javascriptcore-tests and got 0 failures.

[Darin Adler]

Comment on attachment 411692 [details]
Patch

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

WebKit project bug fixes require including a test with the patch.

Given this fixes a test, why is there no change to test expectations showing the newly passing test?

> Source/JavaScriptCore/parser/Parser.cpp:4032
> +    bool oldAllowsIn = m_allowsIn;
> +    m_allowsIn = true;
>      TreeExpression lhs = parseAssignmentExpression(context);
> +    m_allowsIn = oldAllowsIn;

Looks fine to me, but there are two other things we sometimes use in cases like this:

1) SetForScope, so we don’t need to do the restore explicitly, but requires a scope.
2) std::exchange so we can get and set something at the same time.

[Alexey Shvayka]

Comment on attachment 411692 [details]
Patch

That's a great first patch, Mark!

r=me with 2 comments:

1. Let's AllowInOverride instead.
2. Please remove now passing test from JSTests/test262/expectations.yaml.

[Mark Cohen]

Created attachment 411722 [details]
Patch

[Mark Cohen]

thanks all for the comments! just uploaded an updated patch. let me know if there's anything else I should address.

[Alexey Shvayka]

Comment on attachment 411722 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:4029
> +    TreeExpression lhs;

The final nit: let's avoid calling default constructor by doing `TreeExpression lhs = 0`, which seems like a convention in Parser.cpp.
Please re-upload the patch with --no-review, since it was already reviewed, and with --request-commit so a WebKit committer would help you land it.

[Mark Cohen]

Created attachment 411758 [details]
Patch

[Mark Cohen]

done and done. thanks!

[Alexey Shvayka]

Comment on attachment 411758 [details]
Patch

Congratulations on your first patch landed! Great job, Mark!

[EWS]

Downloading keyring-7.3.1...
Installing keyring-7.3.1...
Installed keyring-7.3.1!
ChangeLog entry in JSTests/ChangeLog contains OOPS!.

[Alexey Shvayka]

(In reply to EWS from comment #11)
> ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Hmm, I guess reviewer's name is required to be filled manually.

[Darin Adler]

Comment on attachment 411758 [details]
Patch

Lets try that again.

[Darin Adler]

I set review+ to try again.

It might be the "wrong" reviewer name (mine), but should work.

[EWS]

Committed r268688: <https://trac.webkit.org/changeset/268688>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411758 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70455906>