[webkit-reviews] review granted: [Bug 29034] ARM compiler does not understand reinterpret_cast<void*> : [Attachment 41086] 2nd try

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Laszlo Gombos
<laszlo.1.gombos at nokia.com>'s request for review:
Bug 29034: ARM compiler does not understand reinterpret_cast<void*>
https://bugs.webkit.org/show_bug.cgi?id=29034

Attachment 41086: 2nd try
https://bugs.webkit.org/attachment.cgi?id=41086&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks much better!

> +	   Change reinterpret_cast<void*> to regular C style (void*) cast 
> +	   for the ARM RVCT compiler.
> +
> +	   * assembler/MacroAssemblerCodeRef.h:
> +	   (JSC::FunctionPtr::FunctionPtr):
> +	   * jit/JITOpcodes.cpp:
> +	   * jit/JITStubCall.h:
> +	   (JSC::JITStubCall::JITStubCall):
> +	   * jit/JITStubs.cpp:
> +	   (JSC::DEFINE_STUB_FUNCTION):

I believe this patch does not change the DEFINE_STUB_FUNCTION macro, nor is a
macro something that's inside a namespace, so this is wrong. Please keep in
mind that prepare-ChangeLog helps you write the change log, but you need to
read it over and correct it if it's wrong. I think that per-file and
per-function comments help make things clearer and I wish you would write
those.

> +#if COMPILER(RVCT)
> +	   : m_value((void*)(value))
> +#else
>	   : m_value(reinterpret_cast<void*>(value))
> +#endif

This needs a comment. It's not obvious why the compiler is relevant here.

> -    move(ImmPtr(reinterpret_cast<void*>(ctiVMThrowTrampoline)), regT2);
> +    move(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value()), regT2);

I'm not sure I understand why you used value() here and executableAddress()
elsewhere.

>  #include <wtf/Platform.h>
>  
> +#include "MacroAssemblerCodeRef.h"

Since MacroAssemblerCodeRef.h already includes Platform.h you should replace
the existing include with it rather than keeping the old include around.

> -	       m_jit->m_calls.append(CallRecord(call, m_jit->m_bytecodeIndex,
m_stub));
> +	       m_jit->m_calls.append(CallRecord(call, m_jit->m_bytecodeIndex,
m_stub.value()));

Same comment here.

I'm going to say review+ with this, but it might be better to address the
issues I mentioned first instead of landing it as-is.