Bug 43464 – Remove the global object from the bytecode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Nathan Lawrence]

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.

[WebKit Review Bot]

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.

[Darin Adler]

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

[Nathan Lawrence]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[Nathan Lawrence]

Created attachment 63492 [details]
patch

forgot the space between swtich and '('

[WebKit Commit Bot]

Comment on attachment 63492 [details]
patch

Clearing flags on attachment: 63492

Committed r64790: <http://trac.webkit.org/changeset/64790>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.