[webkit-reviews] review denied: [Bug 25832] Refactor JIT code-handle objects. : [Attachment 30410] Fix bdash's review comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 25832: Refactor JIT code-handle objects.
https://bugs.webkit.org/show_bug.cgi?id=25832

Attachment 30410: Fix bdash's review comments.
https://bugs.webkit.org/attachment.cgi?id=30410&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   wrap teh code pointer with a RefPtr to the pool.  To add to the
mire, as well as the

"teh"

> +	      
AssemblerType::patchMacroAssemblerCall(reinterpret_cast<intptr_t>(this->m_locat
ion), reinterpret_cast<void*>(label.getJumpDestination()));

Why the "this->" here?

> +	   CodeRef()
> +	       : m_code(0)
> +	   {
> +	   }

Should this initialize size to 0 in debug versions?

> +	   void* m_code;
> +	   RefPtr<ExecutablePool> m_executablePool;
> +#ifndef NDEBUG
> +	   size_t m_size;
> +#endif

In the past we've normally made struct members have no prefix, and used the m_
prefix for private class members. Any reason to not just make this a class? Or
you could remove the m_ prefixes? Or just leave as is if you like.

Would it be OK to copy a CodeRef? If not, then I suggest inheriting from
Noncopyable.

> +	   template <class AssemblerType_T>

The _T in the name is not something we do anywhere else. I think you should
just omit it.

I suggest making PatchBuffer inherit from Noncopyable too. Why doesn't
PatchBuffer just have a private CodeRef data member?

> +	   inline void complete()
> +	   {
> +#ifndef NDEBUG
> +	       m_completed = true;
> +#endif
> +	   }

No need for the "inline" here. Also, this doesn't need to go before the uses of
it. Within a class definition, all the compilers handle things in any order.

I think you should put the ASSERT(!m_completed) inside the complete() function.
I think the complete() function needs a different name such as
markAsComplete(). The ambiguity about complete which could be either a verb or
adjective makes it not such a good name.

> +	   CodeRef finalize()
> +	   {
> +	       ASSERT(!m_completed);
> +	       complete();
> +
> +	       return CodeRef(m_code, m_executablePool, m_size);
> +	   }
> +	   CodeLocationLabel entry()
> +	   {
> +	       ASSERT(!m_completed);
> +	       complete();
> +
> +	       return CodeLocationLabel(m_code);
> +	   }

It's a bit weak to name the function entry() when it's an action with a side
effect. Also, I don't think you need to explicitly call the constructor
CodeLocationLabel -- implicit conversion will take care of it.

> +#if ENABLE(JIT)
> +#endif

You should delete this.

> +	   void setJITCode(JITCode jitCode);

No need for the argument name jitCode here.

> -	   static void linkCall(JSFunction* callee, CodeBlock* calleeCodeBlock,
JITCode ctiCode, CallLinkInfo* callLinkInfo, int callerArgCount);
> +	   static void linkCall(JSFunction* callee, CodeBlock* calleeCodeBlock,
JITCode&, CallLinkInfo* callLinkInfo, int callerArgCount);

No need for the argument name callLinkInfo here. And I do hate "info".

> +#include "MacroAssembler.h"
>  #include "CallFrame.h"
>  #include "JSValue.h"
>  #include "Profiler.h"

Please keep include sorted alphabetically.

> +	   JITCode(MacroAssembler::CodeRef ref)
> +	       : m_ref(ref)
>	   {
>	   }

Maybe the argument should be const MacroAssembler::CodeRef&? You might want to
do this:

    typedef MacroAssembler::CodeRef CodeRef;

Inside this class you could them omit the CodeRef thing.

>	   operator bool() const
>	   {
> -	       return code != 0;
> +	       return m_ref.m_code != 0;
>	   }

You don't need the != 0 here -- bool does that automatically.

It's not good style to have operator bool. You should either use an explicitly
named function if you want to be clean, or use the safe operator bool idiom
from RefPtr.

> +#ifndef NDEBUG
> +	   inline size_t size()
> +	   {
> +	       ASSERT(m_ref.m_code);
> +	       return m_ref.m_size;
> +	   }
> +#endif

It's pointless to have this marked inline for two reasons. First, all functions
declared inside class definitions are always inline. Second, when NDEBUG is not
defined, we don't do any inlining anyway. I don't think there's any good reason
to put this function inside an #if -- what's wrong with asking the size outside
debugging code?

I'm not really happy about the use of void* for code pointers. Lack of any type
safety means it's unnecessarily easy to pass the wrong argument. I think we
should come up with a scheme for giving executable code a type.

> +	   ExecutablePool* getExecutablePool()
> +	   {
> +	       return m_jitCode.executablePool();
> +	   }

I see a reason to use "get" in the function name here.

> +	   void setJITCode(JITCode jitCode)
> +	   {
> +	       m_jitCode = jitCode;
> +	   }

Should the argument be const JITCode&?


> -    if (jitObject.m_pcreFallback) {
> -	   int result = jsRegExpExecute(jitObject.m_pcreFallback, input,
length, start, output, outputArraySize);
> +    if (jitObject.pcreFallback()) {
> +	   int result = jsRegExpExecute(jitObject.pcreFallback(), input,
length, start, output, outputArraySize);
>	   return (result < 0) ? -1 : output[0];
> -    } else {
> -#if PLATFORM(X86) && !COMPILER(MSVC)
> -	   typedef int (*RegexJITCode)(const UChar* input, unsigned start,
unsigned length, int* output) __attribute__ ((regparm (3)));
> -#else
> -	   typedef int (*RegexJITCode)(const UChar* input, unsigned start,
unsigned length, int* output);
> -#endif
> -	   return reinterpret_cast<RegexJITCode>(jitObject.m_jitCode)(input,
start, length, output);
> -    }
> +    } else
> +	   return jitObject.execute(input, start, length, output);

We normally don't do else after return.

>  
> +
> +#if PLATFORM(X86) && !COMPILER(MSVC)
> +#define YARR_CALL __attribute__ ((regparm (3)))
> +#else
> +#define YARR_CALL
> +#endif
> +
> +

We normally use single blank lines, never double.

> +    JSRegExp* pcreFallback() { return m_pcreFallback; }
> +    void setFallback(JSRegExp* fallback) { m_pcreFallback = fallback; }

Can this just be named fallback? Why "pcreFallback" in the getter but not the
setter?

> +    operator bool() { return m_ref.m_code; }

Same thought as above about operator bool. It's not safe!

I had enough nitpicks that I'll say review-.