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.
Created attachment 411692 [details] Patch
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.
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.
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.
Created attachment 411722 [details] Patch
thanks all for the comments! just uploaded an updated patch. let me know if there's anything else I should address.
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.
Created attachment 411758 [details] Patch
done and done. thanks!
Comment on attachment 411758 [details] Patch Congratulations on your first patch landed! Great job, Mark!
Downloading keyring-7.3.1... Installing keyring-7.3.1... Installed keyring-7.3.1! ChangeLog entry in JSTests/ChangeLog contains OOPS!.
(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.
Comment on attachment 411758 [details] Patch Lets try that again.
I set review+ to try again. It might be the "wrong" reviewer name (mine), but should work.
Committed r268688: <https://trac.webkit.org/changeset/268688> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411758 [details].
<rdar://problem/70455906>