Bug 101013 – Lexer::scanRegExp, create 8 bit pattern and flag Identifiers from 16 bit source when possible

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

scanRegExp() currently creates Identifiers of the same bitness as the source.  Many regular expressions in 16 bit sources consist of only 8 bit characters.

[Michael Saboff]

Created attachment 172135 [details]
Patch

[Darin Adler]

Comment on attachment 172135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172135&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:1689
>  template <typename T>
> +static inline void orCharacter(UChar&, UChar);

I don’t think the static here has any effect anything.

> Source/JavaScriptCore/parser/Lexer.cpp:1692
> +template <>
> +inline void orCharacter<LChar>(UChar&, UChar) { }

Don’t we want static here?

> Source/JavaScriptCore/parser/Lexer.cpp:1695
> +template <>
> +inline void orCharacter<UChar>(UChar& orAccumulator, UChar character)

Don’t we want static here?

> Source/JavaScriptCore/parser/Lexer.cpp:1707
> +    UChar orAllChars = 0;

It’s awkward that “or all chars” is a verb phrase. I’d suggest a noun phrase, "charactersOredTogether" or something like that.

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 172135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172135&action=review
> 
> > Source/JavaScriptCore/parser/Lexer.cpp:1689
> >  template <typename T>
> > +static inline void orCharacter(UChar&, UChar);
> 
> I don’t think the static here has any effect anything.

I think it provide the same file scoping as it would to a normal function declaration.  I'll verify by looking at the exported symbols from the .o file.

> > Source/JavaScriptCore/parser/Lexer.cpp:1692
> > +template <>
> > +inline void orCharacter<LChar>(UChar&, UChar) { }
> 
> Don’t we want static here?

No, the static can't be with the explicit specialization.  The compiler flags an error.
 
> > Source/JavaScriptCore/parser/Lexer.cpp:1695
> > +template <>
> > +inline void orCharacter<UChar>(UChar& orAccumulator, UChar character)
> 
> Don’t we want static here?

No.  Ditto.

> > Source/JavaScriptCore/parser/Lexer.cpp:1707
> > +    UChar orAllChars = 0;
> 
> It’s awkward that “or all chars” is a verb phrase. I’d suggest a noun phrase, "charactersOredTogether" or something like that.

Will do.

[Michael Saboff]

I tried the template declaration with and without the "static".  With the static, the symbols for the two instance are local, but without the static they are global.

[Michael Saboff]

Committed r133668: <http://trac.webkit.org/changeset/133668>