Bug 145935 – Fix minor ES6 compliance issue in RegExp.prototype.toString and optimize performance a little

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

[Darin Adler]

Fix minor ES6 compliance issue in RegExp.prototype.toString and optimize performance a little

[Darin Adler]

My first cut at a patch doesn’t have a regression test because I don’t know where to put it.

[Darin Adler]

Created attachment 254813 [details]
Patch

[Darin Adler]

Comment on attachment 254813 [details]
Patch

Don’t really want to get this reviewed until I add a test for the behavior change.

[Jordan Harband]

LGTM, thanks for cleaning up my patch too!

[Darin Adler]

Created attachment 254814 [details]
Patch

[Darin Adler]

Created attachment 254844 [details]
Patch

[Darin Adler]

Now this has a test. Added a few people who might review the patch.

[Anders Carlsson]

Comment on attachment 254844 [details]
Patch

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

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:140
> +typedef char FlagsString[3 + 1]; // 3 different flags and a null character terminator

I'd use std::array for this.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:142
> +static inline void getFlags(ExecState* exec, JSObject* regexp, FlagsString& flags)

I don't think this needs to use the get pattern - it can just return a FlagsString (if FlagsString were an std::array that is).

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:159
> +    flags[index] = 0;

I'd assert that index < sizeof(FlagsString) here.

[Anders Carlsson]

(In reply to comment #8)

> > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:142
> > +static inline void getFlags(ExecState* exec, JSObject* regexp, FlagsString& flags)
> 
> I don't think this needs to use the get pattern - it can just return a
> FlagsString (if FlagsString were an std::array that is).
> 

I just tested, and if a function returns an std::array<char, 4>, the compiler will just put the entire value in a register - no memory access needed!

[Darin Adler]

Committed r185528: <http://trac.webkit.org/changeset/185528>

[Anders Carlsson]

(In reply to comment #10)
> Committed r185528: <http://trac.webkit.org/changeset/185528>

I think you need to initialize std::array (You can use std::fill for example).

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > Committed r185528: <http://trac.webkit.org/changeset/185528>
> 
> I think you need to initialize std::array (You can use std::fill for
> example).

I don’t think I do.

If there is an exception, the return value is ignored.

If the array is used, we initialize the characters in the string and the null character terminator. We don't need to initialize the rest of the array.

Might need to initialize it if we use some strict memory debugger tool or validator perhaps.