Bug 101013 - Lexer::scanRegExp, create 8 bit pattern and flag Identifiers from 16 bit source when possible
Summary: Lexer::scanRegExp, create 8 bit pattern and flag Identifiers from 16 bit sour...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-01 22:02 PDT by Michael Saboff
Modified: 2022-02-27 23:26 PST (History)
0 users

See Also:


Attachments
Patch (5.06 KB, patch)
2012-11-02 14:25 PDT, Michael Saboff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-11-01 22:02:30 PDT
scanRegExp() currently creates Identifiers of the same bitness as the source.  Many regular expressions in 16 bit sources consist of only 8 bit characters.
Comment 1 Michael Saboff 2012-11-02 14:25:39 PDT
Created attachment 172135 [details]
Patch
Comment 2 Darin Adler 2012-11-06 09:34:58 PST
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.
Comment 3 Michael Saboff 2012-11-06 13:28:48 PST
(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.
Comment 4 Michael Saboff 2012-11-06 13:48:45 PST
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.
Comment 5 Michael Saboff 2012-11-06 14:02:23 PST
Committed r133668: <http://trac.webkit.org/changeset/133668>