[webkit-reviews] review denied: [Bug 207330] Fix code origin when lowering offlineasm instructions on MIPS/ARM64E : [Attachment 395933] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Angelos Oikonomopoulos
<angelos at igalia.com>'s request for review:
Bug 207330: Fix code origin when lowering offlineasm instructions on
MIPS/ARM64E
https://bugs.webkit.org/show_bug.cgi?id=207330

Attachment 395933: Patch

https://bugs.webkit.org/attachment.cgi?id=395933&action=review




--- Comment #7 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 395933
  --> https://bugs.webkit.org/attachment.cgi?id=395933
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395933&action=review

The patch login generally looks good but the names don't describe what is
actually being done.  I suggested replacement names.  r- for now because I
don't think this patch is ready for landing.

> Source/JavaScriptCore/offlineasm/arm64e.rb:57
> +		       newList << Instruction.duplicate_with_operands(operands)

1. Let's use camel case for the function name because we've been using camel
case everywhere else for functions that we define.
2. I suggest calling this cloneWithNewOperands() instead.  I think this is a
better name because it tells us that we're effectively cloning the instruction,
except that we're giving it new operands.  "duplicate" implies that we're
keeping the original.  We're not.  We'll be replacing it: returning wasHandled
true tells the client that it doesn't need to process this instruction and can
skip it, where "processing" this instruction may mean just adding it to the
newList as is.

> Source/JavaScriptCore/offlineasm/ast.rb:921
> +    def duplicate_with_operands(operands)

Let's call this cloneWithNewOperands(), and let's rename the argument to
newOperands.

> Source/JavaScriptCore/offlineasm/mips.rb:560
> +    def mipsAsRegister(preList, postList, operandIndex, needRestore)

How about calling this mipsCloneWithOperandLowered()?

> Source/JavaScriptCore/offlineasm/risc.rb:431
> +def riscOperandAsRegister(insn, preList, postList, operandIndex, suffix,
needStore)

The old riscAsRegister() name wasn't a very good name to begin with.  The new
one isn't better.  Let's call it riscLowerOperandToRegister() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:443
> +def riscOperandsAsRegisters(insn, preList, postList, suffix)

Let's call this riscLowerOperandsToRegisters().

> Source/JavaScriptCore/offlineasm/risc.rb:455
> +    def riscAsRegister(preList, postList, operandIndex, suffix, needStore)

Let's call this riscCloneWithOperandLowered() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:461
> +    def riscAsRegisters(preList, postList, suffix)

Let's call this riscCloneWithOperandsLowered() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:483
> +		   newList << node.riscAsRegisters(newList, postInstructions,
"i")

riscAsRegisters() is a bad name here.  This operation is actually doing 2
things: (1) cloning the instruction, and (2) lowering the operands to registers
based on the suffix.  How about something like riscCloneWithOperandsLowered()?

> Source/JavaScriptCore/offlineasm/risc.rb:498
> +		   newList << node.riscAsRegister(newList, postInstructions, 0,
"p", false)

riscAsRegister() is also a bad name here.  The operation is actually (1)
cloning the instruction with (2) the operand lowered to a register.  How about
calling this riscCloneWithOperandLowered()?