Re: RFR: 8271302: Regex Test Refresh [v2]
On Fri, 20 Aug 2021 13:32:24 GMT, Pavel Rappo wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Couple of fixes
>
> test/jdk/java/util/regex/NegativeArraySize.java line 29:
>
>> 27: * @summary Pattern.compile() can throw confusing
>> NegativeArraySizeException
>> 28: * @requires os.maxMemory >= 5g
>> 29: * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
>
> I note that the order of the arguments has changed. Will that work as
> expected? Had it worked as expected before?
The new order is consistent with other tests. I had difficulty getting it to
run in the original configuration. Perhaps jtreg is more sensitive on order
with the testng runner.
> test/jdk/java/util/regex/RegExTest.java line 121:
>
>> 119: private static void check(String p, String s, boolean expected) {
>> 120: Matcher matcher = Pattern.compile(p).matcher(s);
>> 121: assertSame(matcher.find(), expected);
>
> Why use `assertSame(Object, Object)`? I would expect `assertEquals(boolean,
> boolean)`.
Artifacting because of the use of `==` I'll make it more readable.
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Fri, 20 Aug 2021 13:46:39 GMT, Pavel Rappo wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Couple of fixes
>
> test/jdk/java/util/regex/NegativeArraySize.java line 40:
>
>> 38: @Test
>> 39: public static void testNegativeArraySize() {
>> 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q"
>> + "a".repeat(42 + Integer.MAX_VALUE / 3)));
>
> One observation on this regex. Although the regex looks invalid because `\\Q`
> misses the pairing `\\E`, it can still be compiled (with a reasonable number
> of a's, of course). Moreover, the resulting pattern matches strings in a
> surprising way:
>
>
> jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches()
> $1 ==> true
Maybe that behavior is expected after all. From "Mastering Regular Expressions"
by Jeffrey E.F. Friedl, 3rd Edition, p. 136:
> Literal-text span: `\Q...\E`
>
> First introduced with Perl, the special sequence `\Q...\E` turns off all
> regex meta-characters between them, except for `\E` itself. (If the `\E` is
> omitted, they are turned off until the end of the regex.)
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves wrote:
>> 8271302: Regex Test Refresh
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Couple of fixes
test/jdk/java/util/regex/NegativeArraySize.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, 2021 Oracle and/or its affiliates. All rights
> reserved.
Add comma after 2021, or the copyright headers check won't pass.
test/jdk/java/util/regex/NegativeArraySize.java line 29:
> 27: * @summary Pattern.compile() can throw confusing
> NegativeArraySizeException
> 28: * @requires os.maxMemory >= 5g
> 29: * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
I note that the order of the arguments has changed. Will that work as expected?
Had it worked as expected before?
test/jdk/java/util/regex/NegativeArraySize.java line 40:
> 38: @Test
> 39: public static void testNegativeArraySize() {
> 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q"
> + "a".repeat(42 + Integer.MAX_VALUE / 3)));
One observation on this regex. Although the regex looks invalid because `\\Q`
misses the pairing `\\E`, it can still be compiled (with a reasonable number of
a's, of course). Moreover, the resulting pattern matches strings in a
surprising way:
jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches()
$1 ==> true
test/jdk/java/util/regex/RegExTest.java line 27:
> 25: * @test
> 26: * @summary tests RegExp framework (use -Dseed=X to set PRNG seed)
> 27: * @author Mike McCloskey
What happened to Mike here? :-)
test/jdk/java/util/regex/RegExTest.java line 121:
> 119: private static void check(String p, String s, boolean expected) {
> 120: Matcher matcher = Pattern.compile(p).matcher(s);
> 121: assertSame(matcher.find(), expected);
Why use `assertSame(Object, Object)`? I would expect `assertEquals(boolean,
boolean)`.
test/jdk/java/util/regex/RegExTest.java line 206:
> 204: }
> 205:
> 206: // Regular expression test// Most of the tests are in a file
Mistakenly joined comment lines?
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves wrote: >> 8271302: Regex Test Refresh > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Couple of fixes Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Fri, 13 Aug 2021 20:17:56 GMT, Brent Christian wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Couple of fixes
>
> test/jdk/java/util/regex/RegExTest.java line 3952:
>
>> 3950:
>> 3951: m = Pattern.compile("\\H").matcher(matcherSubstring);
>> 3952: assertTrue(m.find() || ng.equals(m.group()));
>
> Should this be:
> `assertTrue(m.find() && ng.equals(m.group()));`
Good catch. De Morgan's mistake
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
> 8271302: Regex Test Refresh Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Couple of fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/1426e323..0e9fa209 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5092=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5092=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
