Bug 51834 – Enhancement: Add Regexp Debug Compare between JIT and Interpreter

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

[Michael Saboff]

It makes sense to add debug code to compare the results from JIT regular expressions to interpreted regular expressions.

[Michael Saboff]

Created attachment 77850 [details]
Patch Adding Debug Code to Compare RegExp JIT with RegExp Interpreter.

[Gavin Barraclough]

Comment on attachment 77850 [details]
Patch Adding Debug Code to Compare RegExp JIT with RegExp Interpreter.

Hi Michael,

Functionally the code all looks great.  I'd like to suggest three stylistic changes.

 1027 #else
 1028 #define ENABLE_YARR_JIT_DEBUG 0

The ENABLE() macro returns false for flags that aren't defined, so there should be no need to define this id YARR_JIT is not enabled.  I think we should remove this redundant code.  (Explicitly setting the flag to 0 in the block that enables YARR_JIT is good through, to broadcast that the setting exists.)

 97 #if ENABLE(YARR_JIT_DEBUG)
 98             res = JITCode;
 99         else
 100             res = ByteCode;
 101 #else
 102         return JITCode;
 103 #endif

The return on line 102 is mis-indented.  I also think this code would be slightly clearer if you copied the if statement on the line just before this block into both the if and the else.

Finally, I think it would be great to move the #if ENABLE(YARR_JIT_DEBUG) code from RegExp::match out into a new function.  It's great to land this tool, but it would also be nice to keep the actual release code in match nice and clear.

cheers,
G.

[Michael Saboff]

Created attachment 78167 [details]
Updated patch addressing reviewer's concerns

Cleaned up some other extraneous trailing and whole line white space.

[Gavin Barraclough]

Comment on attachment 78167 [details]
Updated patch addressing reviewer's concerns

Looks great Michael

[Michael Saboff]

Committed r75208: <http://trac.webkit.org/changeset/75208>