Bug 51021 - Add Yarr.h to YARR
Summary: Add Yarr.h to YARR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Peter Varga
URL:
Keywords:
Depends on: 52286
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-14 02:20 PST by Peter Varga
Modified: 2011-01-12 06:18 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (18.99 KB, patch)
2010-12-14 02:44 PST, Peter Varga
no flags Details | Formatted Diff | Diff
proposed patch v2 (17.29 KB, patch)
2011-01-11 06:44 PST, Peter Varga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Varga 2010-12-14 02:20:07 PST
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).
Comment 1 Peter Varga 2010-12-14 02:44:47 PST
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.
Comment 2 Gavin Barraclough 2010-12-23 17:19:38 PST
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.
Comment 3 Gavin Barraclough 2010-12-23 17:42:02 PST
Peter,

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

cheers,
G.
Comment 4 Peter Varga 2011-01-04 07:33:37 PST
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".
Comment 5 Peter Varga 2011-01-11 06:44:24 PST
Created attachment 78524 [details]
proposed patch v2
Comment 6 Gavin Barraclough 2011-01-11 12:43:00 PST
Comment on attachment 78524 [details]
proposed patch v2

Looks great!
Comment 7 WebKit Commit Bot 2011-01-11 12:59:22 PST
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
Comment 8 Peter Varga 2011-01-12 02:34:32 PST
Landed in r75595.
Comment 9 Csaba Osztrogonác 2011-01-12 03:09:58 PST
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.
Comment 10 Csaba Osztrogonác 2011-01-12 03:20:04 PST
(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.
Comment 11 Peter Varga 2011-01-12 04:58:27 PST
The removing of numSubpatterns variable from the YarrPattern::compile() function brokes the
fast/regex/pcre-test-1.html.

I fixed it at r75602.