[webkit-reviews] review granted: [Bug 71331] Towards 8 Bit Strings: Templatize JSC::Lexer class by character type : [Attachment 113734] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 71331: Towards 8 Bit Strings: Templatize JSC::Lexer class by character type
https://bugs.webkit.org/show_bug.cgi?id=71331

Attachment 113734: Updated patch
https://bugs.webkit.org/attachment.cgi?id=113734&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=113734&action=review


> Source/JavaScriptCore/parser/Lexer.cpp:290
> +	   if (sourceString->is8Bit()) {
> +	       ASSERT(sizeof(T) == sizeof(LChar));
> +	       data = reinterpret_cast<const T*>(sourceString->characters8());
> +	   } else  {
> +	       ASSERT(sizeof(T) == sizeof(UChar));
> +	       data = reinterpret_cast<const T*>(sourceString->characters16());

> +	   }

If would be better to have the if condition be based on the compile time
condition, and the assertion be the runtime condition, rather than vice versa.

> Source/JavaScriptCore/parser/Lexer.cpp:488
> +template <bool shouldCreateIdentifier> ALWAYS_INLINE JSTokenType
Lexer<LChar>::parseIdentifier(JSTokenData* tokenData, unsigned lexType, bool
strictMode)

Despite the fact that there are some things the 8-bit and 16-bit version of
this function need to do differently, there is still a lot of duplicate code.
Is there a way to factor this so more code is shared? Maybe in the future.

> Source/JavaScriptCore/parser/Lexer.h:68
> +enum LexType {
> +    LexIgnoreReservedWords = 1, 
> +    LexDontBuildStrings = 2,
> +    LexDontBuildKeywords = 4
> +};

I don’t think abbreviating Lexer to Lex here is worthwhile. Also, the existing
name for the enum “LexType”, is not good. It should be “LexerFlag” or something
along those lines.

> Source/JavaScriptCore/parser/Lexer.h:70
> +enum ShiftType { DoBoundsCheck, DoNotBoundsCheck };

The name here, not new, is also not good. This is BoundsCheckPolicy or
something like that, not “shift type”.

> Source/JavaScriptCore/parser/Lexer.h:135
> +    void append16(const UChar* p, size_t length) { m_buffer16.append(p,
length); }

Better to use a word instead of “p”. I’d use the word characters.

> Source/JavaScriptCore/parser/Lexer.h:156
>      template <bool shouldBuildIdentifiers> ALWAYS_INLINE JSTokenType
parseIdentifier(JSTokenData*, unsigned, bool strictMode);
> +    template <bool shouldBuildIdentifiers> NEVER_INLINE JSTokenType
parseIdentifierSlowCase(JSTokenData*, unsigned, bool strictMode);

Not clear what the “unsigned” is. It probably needs an argument name.

> Source/JavaScriptCore/parser/SourceCode.h:78
> -	   SourceCode subExpression(int, int, int);
> +	   SourceCode subExpression(unsigned, unsigned, int);

This needs argument names.

> Source/WebCore/bindings/js/StringSourceProvider.h:67
>      }
> +    
> +    
>  

Why whitespace added?