Fix #3280: Count capturing groups independently on find. #3468
Conversation
|
UPDATE: Nevermind. I have checked generic Privacy Policy again and it actually applies to all sites operated by Lightbend and is pretty clear otherwise. I have signed CLA I am checking out your CLA but I am slightly confused. I guess it wants my real name, but it is not clear whether Ligtbend's "Privacy policy" applies to data gathered in CLA or if CLA is separate and gathered data are never disclosed and used just in case of "problems". Also it's not clear, if Lightbend can access the submitted data since CLA only mentions EPFL but no Lightbend or third-parties (in context of data access) in general, but form is submitted on Lighbend's website. Could you give me answer to at least that part with full name sharing with third-parties? |
|
I did sign the CLA. |
There was a problem hiding this comment.
Thank you for your PR. Very cool fix!
I have a few comments below. Also, could you include the issue being fixed in the commit message, as follows:
Fix #3280: Count capturing groups independently on find.
to follow our commit message guide.
...-suite/shared/src/test/scala/org/scalajs/testsuite/javalib/util/regex/RegexMatcherTest.scala
Show resolved
Hide resolved
| case Some(n) => n | ||
|
|
||
| case None => | ||
| val groupCountPattern = Pattern.compile("|" + pattern0.jsPattern) |
There was a problem hiding this comment.
Before using your idea, it would be nice to test the case if (lastMatch != null), and in that case directly use lastMatch.length - 1. This should hopefully be common case, and will be faster than creating a new RegExp and running a dummy query on it.
|
|
||
| case None => | ||
| val groupCountPattern = Pattern.compile("|" + pattern0.jsPattern) | ||
| val groupCountRegex = groupCountPattern.newJSRegExp() |
There was a problem hiding this comment.
You can avoid the Pattern.compile step in this case. Just do
val groupCountRegex = new js.RegExp("|" + pattern0.jsPattern)this will be faster. Flags are irrelevant, and pattern0.jsPattern has already been processed for in-pattern flags, so Pattern.compile is going to do work for nothing.
Hum, just to make sure if you are concerned about your real name: There is a very real seeming name in your git commit metadata which is publicly visible. |
|
@gzm0 I know. I was just concerned that it wants real name without any privacy agreement (which was oversight on my side). Just because I use my real name on github does not mean that I automatically want to "give" my real name to another company when I don't know what they are going to use it for, especially when CLA actually mentions only EPFL (which I have no problem with). It's purely ideological problem on my side. |
| def groupCount(): Int = ensureLastMatch.length-1 | ||
| def groupCount(): Int = { | ||
| if (lastMatch != null) lastMatch.length-1 | ||
| else { |
There was a problem hiding this comment.
Looking good! One tiny remaining detail: could you format the above as
if (lastMatch != null) {
lastMatch.length-1
} else {
...to follow our if/else coding style which demands that both branches of an if/else must have {}, or none of them has braces.
Good to merge after that!
Since
https://www.ecma-international.org/ecma-262/6.0/#sec-regexpbuiltinexec
points 19 nad 20 are always hit and set result size to NcapturingParens+1 and
https://www.ecma-international.org/ecma-262/6.0/#sec-notation
states that "NcapturingParens is the total number of left capturing parentheses" then it should be safe to create always-matching regexp by prepending "|" and getting group count from resulting successful match.
Fixes #3280