[webkit-reviews] review granted: [Bug 61913] Improve keyword lookup : [Attachment 95977] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 61913: Improve keyword lookup
https://bugs.webkit.org/show_bug.cgi?id=61913

Attachment 95977: Patch
https://bugs.webkit.org/attachment.cgi?id=95977&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=95973&action=review

r=me with these changes

> if ((remaining >= maxTokenLength) && !(lexType & IgnoreReservedWords)) {

WebKit style says this should be an early return (assuming an inline function).


> Source/JavaScriptCore/parser/Lexer.cpp:229
> +#if CPU(NEEDS_ALIGNED_ACCESS)
> +
> +#define COMPARE_CHARACTERS2(address, char1, char2) \
> +    (((address)[0] == char1) && ((address)[1] == char2))
> +#define COMPARE_CHARACTERS4(address, char1, char2, char3, char4) \
> +    (COMPARE_CHARACTERS2(address, char1, char2) &&
COMPARE_CHARACTERS2((address) + 2, char3, char4))

Seems like a lot of these macros could be inline functions instead.

> Source/JavaScriptCore/parser/Lexer.cpp:455
> +#include "KeywordLookup.h"

I think it would be better if KeywordLookup.h defined an inline function
instead. Easier for debugging.

> Source/JavaScriptCore/parser/Lexer.h:116
> +	   template <int shiftAmount, bool shouldBoundsCheck> void
internalShift();

An enum { DoBoundsCheck, DoNotBoundsCheck } would help make call sites more
obvious. A stand-alone "true" or "false" is non-obvious.