WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.32 KB, patch)
2020-10-18 16:56 PDT
,
Mark Cohen
no flags
Details
Formatted Diff
Diff
Patch
(6.32 KB, patch)
2020-10-19 10:06 PDT
,
Mark Cohen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mark Cohen
Comment 1
2020-10-17 18:36:24 PDT
Created
attachment 411692
[details]
Patch
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
Created
attachment 411722
[details]
Patch
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
Created
attachment 411758
[details]
Patch
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
<
rdar://problem/70455906
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug