[webkit-reviews] review granted: [Bug 115827] JSC: There should be a disassembler for ARM Thumb 2 : [Attachment 201126] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 115827: JSC: There should be a disassembler for ARM Thumb 2
https://bugs.webkit.org/show_bug.cgi?id=115827

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201126&action=review


r=me, but do the style fixes and such.

>> Source/JavaScriptCore/disassembler/ARMv7Disassembler.cpp:31
>> +#include "MacroAssemblerCodeRef.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Yeah, you should fix this.

>> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:30
>> +#include "ARMv7DOpcode.h"
> 
> You should add a blank line after implementation file's own header. 
[build/include_order] [4]

Actually, this should be included about the "#if USE(..." and immediately after
"#include "config.h"".

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:46
> +const char* const ARMv7DOpcode::s_optionName[8] =
> +{ "uxtb", "uxth", "uxtw", "uxtx", "sxtb", "sxth", "sxtw", "sxtx" };

This is weird, I would put the '{' at end of the previous line.

>> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:160
>> +	ASSERT (blocksize > 0 && blocksize <= MaxITBlockSize);
> 
> Extra space before ( in function call  [whitespace/parens] [4]

Word.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:221
> +void ARMv7DOpcode::bufferPrintf(const char* format, ...)
> +{
> +    if (m_bufferOffset >= bufferSize)
> +	   return;
> +
> +    va_list argList;
> +    va_start(argList, format);
> +
> +    m_bufferOffset += vsnprintf(m_formatBuffer + m_bufferOffset, bufferSize
- m_bufferOffset, format, argList);
> +
> +    va_end(argList);
> +}

You could use the StringPrintStream.  But this is fine.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:227
> +

Weird whitespace.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:246
> +    if (length >= 7)
> +	   length = 6;

std::min() ?

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:1361
> +	       return m_formatBuffer;
> +	   } else
> +	       appendRegisterName(rn());

I think that stylebot doesn't like you for this else statement.  I agree, it's
weird.