[webkit-reviews] review granted: [Bug 131205] LLInt interpreter code should be generated as part of one function : [Attachment 229066] Patch addressing issues raised in review

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 131205: LLInt interpreter code should be generated as part of one function
https://bugs.webkit.org/show_bug.cgi?id=131205

Attachment 229066: Patch addressing issues raised in review
https://bugs.webkit.org/attachment.cgi?id=229066&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229066&action=review


r=me with fixes.

> Source/JavaScriptCore/ChangeLog:40
> +	   as well as  including files from other than the local directory via
a newly

Extra space between "as  including".

> Source/JavaScriptCore/ChangeLog:45
> +	   Updated the generationo f the masm only .sym file to be written once
at the end

/generationo f/generation of/.

> Source/JavaScriptCore/ChangeLog:46
> +	   of online assembling.

/online assembling/offline assembly/.

> Source/JavaScriptCore/llint/LLIntData.h:38
> +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS +
NUMBER_OF_CLOOP_BYTECODE_HELPER_IDS + NUMBER_OF_BYTECODE_HELPER_IDS;

This looks identical to the definition of numOpcodeIDs.  I think once upon a
time, these 2 values are different, but now they are redundant.  Let's get rid
of opcodeMapSize and just use numOpcodeIDs everywhere instead.

> Source/JavaScriptCore/llint/LLIntData.h:41
> +const int opcodeMapSize = NUMBER_OF_BYTECODE_IDS +
NUMBER_OF_BYTECODE_HELPER_IDS;

Ditto.

> Source/JavaScriptCore/llint/LLIntData.h:64
> +    friend void* getCodePtr(unsigned);
> +    friend void* getHelperCodePtr(unsigned);

I think these 2 are now obsolete and not referenced anywhere.  Please remove.

> Source/JavaScriptCore/llint/LLIntData.h:118
>  ALWAYS_INLINE void* getCodePtr(JSC::EncodedJSValue glueHelper())

I presume this is still needed for "LLInt::getCodePtr(getHostCallReturnValue))"
or something.  Please confirm that it is still needed.	If not, please remove.

> Source/JavaScriptCore/llint/LowLevelInterpreter.h:50
>  #endif // !ENABLE(LLINT_C_LOOP)

/!ENABLE(LLINT_C_LOOP)/ENABLE(LLINT_C_LOOP)/ since there's no longer a
!ENABLE(LLINT_C_LOOP) case.