| Summary: | test262: test/language/expressions/conditional/in-branch-1.js | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Cohen <m> | ||||||||
| Component: | JavaScriptCore | Assignee: | Mark Cohen <m> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ashvayka, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Mark Cohen
2020-10-17 14:15:34 PDT
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]. |