[webkit-reviews] review granted: [Bug 43464] Remove the global object from the bytecode : [Attachment 63411] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nathan Lawrence
<nlawrence at apple.com>'s request for review:
Bug 43464: Remove the global object from the bytecode
https://bugs.webkit.org/show_bug.cgi?id=43464

Attachment 63411: patch
https://bugs.webkit.org/attachment.cgi?id=63411&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   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