[webkit-reviews] review denied: [Bug 67826] JavaScriptCore does not have speculative->baseline OSR : [Attachment 107000] the patch - fix style

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 67826: JavaScriptCore does not have speculative->baseline OSR
https://bugs.webkit.org/show_bug.cgi?id=67826

Attachment 107000: the patch - fix style
https://bugs.webkit.org/attachment.cgi?id=107000&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107000&action=review


> Source/JavaScriptCore/dfg/DFGDriver.cpp:47
> +    if (false) {
> +	   switch (codeBlock->instructions().size() * sizeof(Instruction)) {
> +	   case 11176:
> +	       return false;
> +	   default:
> +	       break;
> +	   }
> +    }

please to be removing the debugging code? :D

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:103
> +#if DFG_OSR

I'd prefer we used the ENABLE() style macros, e.g. ENABLE(DFG_OSR)

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:109
> +#if DFG_DEBUG_VERBOSE

ENABLE() again

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:113
> +#if DFG_JIT_BREAK_ON_SPECULATION_FAILURE

and again

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:117
> +#if DFG_VERBOSE_SPECULATION_FAILURE

...

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:1184
> +#if DFG_OSR

ENABLE()

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:176
> +#ifndef NDEBUG

This seems more related to the presence or absence of VERBOSE logging rather
than debug build vs. not debug build -- perhaps it should be 
#if ENABLE(....) || !defined(NDEBUG) 
?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:219
> +    JSValue constant() const
> +    {
> +	   ASSERT(m_technique == Constant);
> +	   return JSValue::decode(m_source.constant);
> +    }

Just for reference can a constant be a JSString?  Does this end up relying on
the containing code block for marking?

> Source/JavaScriptCore/runtime/JSGlobalData.h:239
> +	   Vector<int64_t*> osrScratchBuffers;

Honestly I think i'd prefer a Vector of vectors here -- If we really felt it
would be beneficial we could come up with a more sensible "Array" class -> the
absence of range checking in debug builds and the need to manually free the
arrays is just a little icky to me.