Created attachment 63411 [details] patch Currently, the global object is being embedded in the JavaScriptCore bytecode, however since the global object is the same for all opcodes in a code block, we can have the global object just be a member of the associated code block.
Attachment 63411 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/bytecode/CodeBlock.cpp:1461: Missing space before ( in if( [whitespace/parens] [5] JavaScriptCore/bytecode/Opcode.h:191: One space before end of line comments [whitespace/comments] [5] JavaScriptCore/bytecode/Opcode.h:259: Missing space before ( in switch( [whitespace/parens] [5] JavaScriptCore/bytecode/Opcode.h:260: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 63411 [details] patch > - if(vPC[4].u.structure) > - vPC[4].u.structure->deref(); > + if(vPC[3].u.structure) > + vPC[3].u.structure->deref(); There's a missing space after the word if in this code. > - macro(op_end, 2) // end must be the last opcode in the list > + macro(op_end, 2) // end must be the last opcode in the list Why did you add this space? > + switch(opcode) > + { There should be a space after the word "switch" and the brace should be on the same line as the switch. > + #define OPCODE_ID_LENGTHS(id, length) case id: return OPCODE_LENGTH(id); > + FOR_EACH_OPCODE_ID(OPCODE_ID_LENGTHS); > + #undef OPCODE_ID_LENGTHS I'd indent the FOR_EACH_OPCODE one level further so it's in the place where the code would have gone. That semicolon after the macro invocation is not needed or helpful. > + default: > + ASSERT_NOT_REACHED(); > + return 0; Normally in switches like this where each case has a return, we put the default and the ASSERT_NOT_REACHED outside the switch statement so the compiler can warn us if there are any enum values that have no case. In this case it's probably not an issue because we use the macro instead, but it would be nice to follow suit unless there is a reason not to. > + }; There's no need for a semicolon after the switch statement's closing brace. > + } > + How did you test all those different code paths? All these concerns are minor, so I'm going to say r=me
Created attachment 63458 [details] patch (In reply to comment #2) I think I've addressed all of your concerns. > How did you test all those different code paths? I compiled and ran javascriptcore tests on the 32 bit JIT, the 64 bit JIT and the 64 bit interpreter.
Attachment 63458 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/bytecode/Opcode.h:259: Missing space before ( in switch( [whitespace/parens] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 63411 [details] patch Cleared Darin Adler's review+ from obsolete attachment 63411 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 63492 [details] patch forgot the space between swtich and '('
Comment on attachment 63492 [details] patch Clearing flags on attachment: 63492 Committed r64790: <http://trac.webkit.org/changeset/64790>
All reviewed patches have been landed. Closing bug.