[Webkit-unassigned] [Bug 29034] ARM compiler does not understand reinterpret_cast<void*>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29034


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41086|review?                     |review+
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2009-10-13 08:54:19 PDT ---
(From update of attachment 41086)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.