[webkit-reviews] review denied: [Bug 198604] Refactoring of architectural Register Information : [Attachment 373008] Post-5 Review Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Paulo Matos
<pmatos at igalia.com>'s request for review:
Bug 198604: Refactoring of architectural Register Information
https://bugs.webkit.org/show_bug.cgi?id=198604

Attachment 373008: Post-5 Review Patch

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




--- Comment #60 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 373008
  --> https://bugs.webkit.org/attachment.cgi?id=373008
Post-5 Review Patch

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

Sorry about the delay. I thought this had already been review and committed for
some reason... :/

I have a couple more style changes for you but I think it's almost there!
Thanks again for the patch.

> Source/JavaScriptCore/assembler/RegisterInfo.h:59
> +#  include "X86Registers.h"
> +#elif CPU(X86_64)
> +#  include "X86_64Registers.h"
> +#elif CPU(MIPS)
> +#  include "MIPSRegisters.h"
> +#elif CPU(ARM_THUMB2)
> +#  include "ARMv7Registers.h"
> +#elif CPU(ARM64)
> +#  include "ARM64Registers.h"

These includes should not have spaces between the # and the "include"

> Source/JavaScriptCore/assembler/X86_64Registers.h:92
> +    macro(xmm10,"xmm10", 0, 0)		   \
> +    macro(xmm11,"xmm11", 0, 0)		   \
> +    macro(xmm12,"xmm12", 0, 0)		   \
> +    macro(xmm13,"xmm13", 0, 0)		   \
> +    macro(xmm14,"xmm14", 0, 0)		   \
> +    macro(xmm15,"xmm15", 0, 0)

Can you add a space after these register identifiers? That would match webkit
style.

> Source/JavaScriptCore/jit/RegisterSet.cpp:52
> +    if (isReserved) result.set(RegisterNames::id);

I think the body of the if should be on its own line and be indented.

> Source/JavaScriptCore/jit/RegisterSet.cpp:116
> +    result.set(RegisterNames::id);

Can you fix the indentation here?