[webkit-reviews] review granted: [Bug 123182] Change local variable register allocation to start at offset -1 : [Attachment 215421] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 123182: Change local variable register allocation to start at offset -1
https://bugs.webkit.org/show_bug.cgi?id=123182

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215421&action=review


> Source/JavaScriptCore/bytecode/VirtualRegister.h:77
> +    static int localToOperand(int local) { return -1-local; }
> +    static int operandToLocal(int operand) { return -1-operand; }

Spaces around "-", please.

Looks like this abstraction worked out really well. It's nice that we could
avoid making any changes to JSActivation, Arguments, and BytecodeGenerator in
this patch.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:320
> +    end = callFrame->registers() + JSStack::ThisArgument;
> +    while (it >= end) {

In C++, "end" means "past the end", so we should keep >, and change the value
of end.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:936
> +    if (((intptr_t)callFrame & 0xfff) == 0xce8)

Please remove.

> Source/JavaScriptCore/interpreter/JSStack.h:56
> +	       ThisArgument = 6,

Let's use avoid duplication with CallFrame:s_thisArgumentOffset and
CallFrame::s_firstArgumentOffset, and let's make sure these constants live next
to each other. If we want them in JSStack.h, I'd call them:

FirstArgument = 7
ThisArgument = 6

> Source/JavaScriptCore/runtime/JSGlobalObject.h:149
>      // Add one so we don't need to index with -1 to get current frame
pointer.
>      // An index of -1 is an error for some compilers.
> -    Register m_globalCallFrame[JSStack::CallFrameHeaderSize + 1];
> +    Register m_globalCallFrame[JSStack::CallFrameHeaderSize];

Please update this comment, or remove it.