Bug 101003 – RegExp.prototype.toString Should Produce an 8 bit JSString if possible.

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

[Michael Saboff]

regExpObjectSource() in RegExpObject.cpp uses a StringBuilder appending UChar characters of the source regular expression when creating the string to return via RegExp.prototype.toString.  This will always create a 16 bit string.  The code should be changed to create an 8 bit string where possible.

[Michael Saboff]

Created attachment 171964 [details]
Patch

[Early Warning System Bot]

Comment on attachment 171964 [details]
Patch

Attachment 171964 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14685813

[Early Warning System Bot]

Comment on attachment 171964 [details]
Patch

Attachment 171964 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14703005

[Darin Adler]

Comment on attachment 171964 [details]
Patch

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

> Source/JavaScriptCore/runtime/RegExpObject.cpp:258
> +            else if (sizeof(CharacterType) > 1) {
> +                if (ch == 0x2028)
> +                    result.appendLiteral("u2028");
> +                else
> +                    result.appendLiteral("u2029");
> +            }

I don’t think this technique is going to work portably; we’ll get warnings on some compilers. I think we need a separate inline function for this that we overload for the 8-bit and 16-bit character types.

Patch otherwise looks fine.

[Michael Saboff]

Created attachment 172096 [details]
Factored out the line terminator escape logic to fix qt builds

(In reply to comment #4)
> (From update of attachment 171964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171964&action=review
> 
> > Source/JavaScriptCore/runtime/RegExpObject.cpp:258
> > +            else if (sizeof(CharacterType) > 1) {
> > +                if (ch == 0x2028)
> > +                    result.appendLiteral("u2028");
> > +                else
> > +                    result.appendLiteral("u2029");
> > +            }
> 
> I don’t think this technique is going to work portably; we’ll get warnings on some compilers. I think we need a separate inline function for this that we overload for the 8-bit and 16-bit character types.

I saw this last night as it broke the qt builds.  I factored out as suggested.

> Patch otherwise looks fine.

[Geoffrey Garen]

Comment on attachment 172096 [details]
Factored out the line terminator escape logic to fix qt builds

r=me

[WebKit Review Bot]

Comment on attachment 172096 [details]
Factored out the line terminator escape logic to fix qt builds

Clearing flags on attachment: 172096

Committed r133333: <http://trac.webkit.org/changeset/133333>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 172096 [details]
Factored out the line terminator escape logic to fix qt builds

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

> Source/JavaScriptCore/runtime/RegExpObject.cpp:182
> +template <typename CharacterType>
> +static inline void appendLineTerminatorEscape(StringBuilder&, CharacterType);

I suggest we just use function overloading instead of templates for these two functions. Should be a simple change: Just remove all the template stuff and put "static" before "inline".