-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Named group support. #49
Conversation
142288c
to
0a6862d
Compare
@@ -980,6 +983,7 @@ private Regexp parseInternal() throws PatternSyntaxException { | |||
if (n != 1) { | |||
throw new PatternSyntaxException(ERR_MISSING_PAREN, wholeRegexp); | |||
} | |||
stack.get(0).namedGroups = namedGroups; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong.
This is what they do in c++:
https://github.com/google/re2/blob/master/re2/regexp.cc#L604-L650
This is what I ported in Scala
scala-native/re2s@9a40319#diff-3a6eea39d89568034dd070ba707f60d9R236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's wrong, just different from the C++ or Scala impls of RE2. The approach in my change appears to be similar to that in the Java SDKs, see http://www.docjar.com/html/api/java/util/regex/Pattern.java.html around line 2800.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjamesr you cannot look at the code in OpenJDK. It's Licensed under GPL. This project is Licensed under this custom License. https://github.com/google/re2j/blob/master/LICENSE
Missing: |
0a6862d
to
b11e55f
Compare
Added |
assertEquals(-1, m.start("nomatch")); | ||
assertEquals(-1, m.end("nomatch")); | ||
assertEquals("whatbbarrrrreverbag", | ||
appendReplacement(m, "what$2ever${bag}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not using the named groups <baz>
or <foo>
above.
0c20b6c
to
b5f9533
Compare
3b94f3e
to
384da93
Compare
I just wanted to mention that I would love to see support for named groups in RE2J. Would be awesome this pull request could be finished and merged :-) |
I'd like somebody who knows about RE2J internals to take a look before I
merge it. I think it's ready for review.
…On Thu, Sep 13, 2018 at 12:44 PM Jarek Jarcec Cecho < ***@***.***> wrote:
I just wanted to mention that I would love to see support for named groups
in RE2J. Would be awesome this pull request could be finished and merged :-)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#49 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEbTA-skfuP8C9cvssT1dAqf2VkStmAzks5uarWbgaJpZM4Sbln6>
.
|
I sadly do not know the internals, so I can't help much with the actual review myself :-( |
80321e4
to
c4b8915
Compare
c4b8915
to
b4e9bef
Compare
b4e9bef
to
e6ad55f
Compare
e6ad55f
to
db421b5
Compare
This changes the parser to annotate the returned Regexp with information about named capture groups. During compliation, this information is passed along to the RE2 instance and thence to the Matcher. The parser will throw PatternSyntaxException if duplicate group names are specified.
db421b5
to
d4fa1e1
Compare
Thank you @sjamesr for merging this change! I’m looking forward to take advantage of it. |
This changes the parser to annotate the returned Regexp with information
about named capture groups. During compliation, this information is
passed along to the RE2 instance and thence to the Matcher.