Bug 51021 – Add Yarr.h to YARR

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

[Peter Varga]

This modification provides a refactoring in the YARR's structure:
- Some unnecessary header includes are removed
- The hierarchy of the includes becomes clearer
- The JSRegExp.h offers an interface thus just one header has to be included to call the YARR's common functions (eg.: compile, execute)
- Most of the common constants of YARR are in one header

I think this modification makes the YARR structure clearer and to use YARR from outside of JSC you only need to include one header (eg.: in WebCore).

[Peter Varga]

Created attachment 76520 [details]
proposed patch

This patch causes a style error:

JavaScriptCore/yarr/RegexCompiler.cpp:29:  Found other header before a header this file implements. 
Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

The reason is the RegexCompiler.h contained just the decleration of the compileRegex() function. I move it to the 
new header: JSRegExp.h. I don't want to move any line from the RegexCompiler.cpp to the RegexCompiler.h 
therefore it became unnecessary.

[Gavin Barraclough]

Hi Peter,

I can see a role for a new header here, and we may want to move some of the existing code, but I'm not sure this change is quite right.

(1) Moving the compile method irks me, because I think we should just delete this – it's only ever, and must always be, called on a new pattern – it should just be a part of pattern object construction! (and the pattern object only exists in order to compile the regexp).  The call to compile (i.e. populate the RegexPattern object) should just be a part of RegexPattern construction.

I hope you don't mind, but I think I'm going to fix this, which will conflict with this patch.

(2) The new file name seems a little inconsistent with convention (not that we're completely consistent unfortunately :-/ ).  We most commonly apply the JS prefix to garbage collected JS object types, for example JSObject, JSString, JSArray.  Yarr should ideally be usable as a standalone library – as pcre is, for uses other than within javascript (i.e., possibly used form WebCore, replacing current PCRE use?), and with no particular connection to JS other than the fact the engine implements the JS regex syntax.  As such, I think it may be best to avoid JS in the file names or interface.  Since the rest of the files in the directory currently use the name Regex, I'd suggest a new header containing common types could just be RegexTypes.h, or maybe just Regex.h.

However it's possible that it was a mistake to use the term Regex in all these class & filenames (since JS generally uses the term RegExp).  It might be less confusing to rename the existing files & classes, replacing "Regex" with "RegExp", or with "Yarr".  If we did so, then I imagine a new header would want to be "Yarr.h", "YarrTypes.h", or "RegexTypes.h"

cheers,
G.

[Gavin Barraclough]

Peter,

I've removed RegexCompiler.cpp/.h in r74600, and made RegexPattern's constructor implicitly populate itself.

cheers,
G.

[Peter Varga]

Hi Gavin,

I was thinking about your advices to a potential renaming issue.

I think the most sensible solution is replace the "Regex" prefix with "Yarr", thus I created a bug report: https://bugs.webkit.org/show_bug.cgi?id=51872

If this solution is acceptable then the new header name should be "Yarr.h".

[Peter Varga]

Created attachment 78524 [details]
proposed patch v2

[Gavin Barraclough]

Comment on attachment 78524 [details]
proposed patch v2

Looks great!

[WebKit Commit Bot]

Comment on attachment 78524 [details]
proposed patch v2

Rejecting attachment 78524 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2
Last 500 characters of output:
re/yarr/YarrPattern.cpp
patching file Source/JavaScriptCore/yarr/YarrPattern.h
(Stripping trailing CRs from patch.)
patching file Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
Hunk #1 FAILED at 1658.
1 out of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Gavin Barraclough', u'..." exit_code: 1

Full output: http://queues.webkit.org/results/7343376

[Peter Varga]

Landed in r75595.

[Csaba Osztrogonác]

It was rolled out, because it broke fast/regex/pcre-test-1.html:
-PASS regex110.exec(input0); is results
+FAIL regex110.exec(input0); should be abcdefghijkllS,a,b,c,d,e,f,g,h,i,j,k,l.

[Csaba Osztrogonác]

(In reply to comment #9)
> It was rolled out, because it broke fast/regex/pcre-test-1.html:
> -PASS regex110.exec(input0); is results
> +FAIL regex110.exec(input0); should be abcdefghijkllS,a,b,c,d,e,f,g,h,i,j,k,l.

-PASS regex110.exec(input0); is results
+FAIL regex110.exec(input0); should be abcdefghijkllS,a,b,c,d,e,f,g,h,i,j,k,l. Was null.

[Peter Varga]

The removing of numSubpatterns variable from the YarrPattern::compile() function brokes the
fast/regex/pcre-test-1.html.

I fixed it at r75602.