RESOLVED FIXED 217879
test262: test/language/expressions/conditional/in-branch-1.js
https://bugs.webkit.org/show_bug.cgi?id=217879
Summary test262: test/language/expressions/conditional/in-branch-1.js
Mark Cohen
Reported 2020-10-17 14:15:34 PDT
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.
Attachments
Patch (4.33 KB, patch)
2020-10-17 18:36 PDT, Mark Cohen
no flags
Patch (6.32 KB, patch)
2020-10-18 16:56 PDT, Mark Cohen
no flags
Patch (6.32 KB, patch)
2020-10-19 10:06 PDT, Mark Cohen
no flags
Mark Cohen
Comment 1 2020-10-17 18:36:24 PDT
Mark Cohen
Comment 2 2020-10-17 18:41:57 PDT
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 3 2020-10-18 13:32:00 PDT
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 4 2020-10-18 14:04:30 PDT
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
Comment 5 2020-10-18 16:56:58 PDT
Mark Cohen
Comment 6 2020-10-18 16:58:10 PDT
thanks all for the comments! just uploaded an updated patch. let me know if there's anything else I should address.
Alexey Shvayka
Comment 7 2020-10-19 04:54:21 PDT
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
Comment 8 2020-10-19 10:06:31 PDT
Mark Cohen
Comment 9 2020-10-19 10:06:58 PDT
done and done. thanks!
Alexey Shvayka
Comment 10 2020-10-19 12:22:02 PDT
Comment on attachment 411758 [details] Patch Congratulations on your first patch landed! Great job, Mark!
EWS
Comment 11 2020-10-19 12:22:41 PDT
Downloading keyring-7.3.1... Installing keyring-7.3.1... Installed keyring-7.3.1! ChangeLog entry in JSTests/ChangeLog contains OOPS!.
Alexey Shvayka
Comment 12 2020-10-19 12:31:02 PDT
(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 13 2020-10-19 12:31:45 PDT
Comment on attachment 411758 [details] Patch Lets try that again.
Darin Adler
Comment 14 2020-10-19 12:32:33 PDT
I set review+ to try again. It might be the "wrong" reviewer name (mine), but should work.
EWS
Comment 15 2020-10-19 14:14:09 PDT
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
Comment 16 2020-10-19 14:15:19 PDT
Note You need to log in before you can comment on or make changes to this bug.