Summary: | JSTests/slowMicrobenchmarks/spread-small-array.js fails with JSC_validateExceptionChecks=1 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Morisset <rmorisset> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Trivial | CC: | buildbot, commit-queue, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Robin Morisset
2017-08-08 15:10:46 PDT
Created attachment 317626 [details]
Proposed patch
Attachment 317626 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ChangeLog:4: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881: Missing space after , [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883: Missing space after , [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889: Missing space after , [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922: Missing space after , [whitespace/comma] [3]
Total errors found: 10 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317631 [details]
Same patch, with the whitespace issues fixed
Comment on attachment 317631 [details] Same patch, with the whitespace issues fixed View in context: https://bugs.webkit.org/attachment.cgi?id=317631&action=review r=me with changelog fix > Source/JavaScriptCore/ChangeLog:10 > + This is done by making finishCreation explicitely check for exceptions after setConstantRegister and setConstantIdentifiersSetRegisters. > + I chose to have this check replace the boolean returned previously by these functions for readability. The performance impact should be > + negligible considering how much more finishCreation does. > + This fix then caused another issue to appear as it was now clear that finishCreation can throw. And since it is called by ProgramCodeBlock::create(), > + FunctionCodeBlock::create() and friends, that are in turn called by ScriptExecutable::newCodeBlockFor, this last function also required a few tweaks. Style nit: This explanation should go below the "Reviewed by ..." Attachment 317631 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922: Extra space before ) [whitespace/parens] [2]
Total errors found: 4 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317634 [details]
Same patch, with Changelog fix
Attachment 317634 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922: Extra space before ) [whitespace/parens] [2]
Total errors found: 4 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317634 [details] Same patch, with Changelog fix View in context: https://bugs.webkit.org/attachment.cgi?id=317634&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:881 > + RETURN_IF_EXCEPTION(scope, ); This looks weird. Let's do this instead: RETURN_IF_EXCEPTION(scope, void()); > Source/JavaScriptCore/bytecode/CodeBlock.cpp:883 > + RETURN_IF_EXCEPTION(scope, ); Ditto. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:889 > + RETURN_IF_EXCEPTION(scope, ); Ditto. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:922 > + RETURN_IF_EXCEPTION(scope, ); Ditto. Created attachment 317638 [details]
Same patch, with Mark's suggestion
Created attachment 317641 [details]
exception_scope => exceptionScope, as suggested by Mark
Comment on attachment 317641 [details] exception_scope => exceptionScope, as suggested by Mark View in context: https://bugs.webkit.org/attachment.cgi?id=317641&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:404 > + auto exceptionScope = DECLARE_THROW_SCOPE(vm); can we call this throwScope instead since it comes from DECLARE_THROW_SCOPE? Created attachment 317648 [details]
exceptionScope => throwScope
Comment on attachment 317648 [details] exceptionScope => throwScope Clearing flags on attachment: 317648 Committed r220432: <http://trac.webkit.org/changeset/220432> All reviewed patches have been landed. Closing bug. |