Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v6]
On Wed, 27 Sep 2023 14:13:05 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments Thanks everyone for prompt reviews! Could someone sponsor this? I also intend to backport to 21u soon. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1739739777
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v6]
On Wed, 27 Sep 2023 14:13:05 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments (not an official Reviewer) - Marked as reviewed by rgiulietti (Committer). PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1647271808
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v6]
On Wed, 27 Sep 2023 14:13:05 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments Looks good. Thanks for the junit and unicode encoding changes. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1646792444
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v6]
> test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15906/files - new: https://git.openjdk.org/jdk/pull/15906/files/caaa99db..aa5faeac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=04-05 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 13:27:10 GMT, Roger Riggs wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> address review comments
>
> test/jdk/java/lang/String/RegionMatches.java line 41:
>
>> 39:
>> 40: private final byte[] b1_UTF16 = new byte[]{0x04, 0x3d, 0x04, 0x30,
>> 0x04, 0x36, 0x04, 0x34};
>> 41: private final byte[] b2_UTF16 = new byte[]{0x04, 0x32, 0x00, 0x20,
>> 0x04, 0x41, 0x04, 0x42};
>
> For strings, the \u version would be preferred; it is clearer that what
> the character is and there is less of a chance that the UTF encoding has a
> mistake.
Done!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1338669536
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 09:58:58 GMT, Aleksei Voitylov
wrote:
>> test java.lang.String.RegionMatches1Tests fails on all platforms with
>> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by
>> default. The fix is to return true immediately if len is negative, since for
>> negative length this condition will never be satisfied.
>>
>> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64
>> and on ARM32.
>
> Aleksei Voitylov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> address review comments
Changes requested by rriggs (Reviewer).
test/jdk/java/lang/String/RegionMatches.java line 41:
> 39:
> 40: private final byte[] b1_UTF16 = new byte[]{0x04, 0x3d, 0x04, 0x30,
> 0x04, 0x36, 0x04, 0x34};
> 41: private final byte[] b2_UTF16 = new byte[]{0x04, 0x32, 0x00, 0x20,
> 0x04, 0x41, 0x04, 0x42};
For strings, the \u version would be preferred; it is clearer that what the
character is and there is less of a chance that the UTF encoding has a mistake.
-
PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1646531054
PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1338610081
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 09:58:58 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments It avoids the two `String` constructor invocations, otherwise it is as (badly) readable as with the `byte[]`. My question was just curiosity. The best thing would be to use `"нажд"` and `"в ст"`, but AFAIK that's not allowed in OpenJDK. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1737365014
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 09:58:58 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments \u0XXX\u0XXX\u0XXX just seems harder to read, but maybe it's just me. I don't have a strong opinion on this, let me know if you believe what you suggest somehow improves readability or is more standard. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1737333440
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 09:58:58 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments Just out of curiosity: why not write, e.g., private final String s1_UTF16 = "\u043d\u0430\u0436\u0434"; private final String s2_UTF16 = "\u0432\u0020\u0441\u0442"; - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1737303082
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
On Wed, 27 Sep 2023 09:58:58 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments Marked as reviewed by rgiulietti (Committer). LGTM (but I'm not an official Reviewer) - PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1646155823 PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1737140274
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v5]
> test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15906/files - new: https://git.openjdk.org/jdk/pull/15906/files/84965be4..caaa99db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=03-04 Stats: 9 lines in 1 file changed: 1 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v4]
On Tue, 26 Sep 2023 16:23:58 GMT, Roger Riggs wrote: >> Aleksei Voitylov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> address review comments > >> test/jdk/java/lang/String/RegionMatches.java was converted to testng and >> split into two @tests, the latter now covers 8316879. > > Can the test use JUnit 5? JUnit is being maintained compared to TestNG. I second @RogerRiggs' note that JUnit is better maintained than TestNG. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1736032210
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v4]
On Tue, 26 Sep 2023 16:18:09 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > address review comments > test/jdk/java/lang/String/RegionMatches.java was converted to testng and > split into two @tests, the latter now covers 8316879. Can the test use JUnit 5? JUnit is being maintained compared to TestNG. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1735880178
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v3]
On Tue, 26 Sep 2023 14:29:05 GMT, Roger Riggs wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.base/share/classes/java/lang/String.java
>>
>> Co-authored-by: Raffaello Giulietti
>
> src/java.base/share/classes/java/lang/String.java line 2162:
>
>> 2160: if (len < 0) {
>> 2161:return true;
>> 2162: }
>
> Isn't it also true that the regions trivial match if len == 0?
Yes, you are right. The statement "there is some nonnegative integer k less
than len " covers the case len == 0, and the rest of the cases are checked
before this line.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1337471588
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v4]
> test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15906/files - new: https://git.openjdk.org/jdk/pull/15906/files/7a505e0d..84965be4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v3]
On Tue, 26 Sep 2023 14:06:56 GMT, Aleksei Voitylov
wrote:
>> test java.lang.String.RegionMatches1Tests fails on all platforms with
>> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by
>> default. The fix is to return true immediately if len is negative, since for
>> negative length this condition will never be satisfied.
>>
>> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64
>> and on ARM32.
>
> Aleksei Voitylov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/String.java
>
> Co-authored-by: Raffaello Giulietti
src/java.base/share/classes/java/lang/String.java line 2162:
> 2160: if (len < 0) {
> 2161:return true;
> 2162: }
Isn't it also true that the regions trivial match if len == 0?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1337310557
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v2]
Since the tests are now TestNG, I think it would make more sense to use TestNG `assertTrue` rather than using explicit `if`s and `throw`s. On 2023-09-26 16:09, Aleksei Voitylov wrote: On Tue, 26 Sep 2023 12:38:46 GMT, Aleksei Voitylov wrote: test java.lang.String.RegionMatches1Tests fails on all platforms with -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by default. The fix is to return true immediately if len is negative, since for negative length this condition will never be satisfied. Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: add regression test @rgiulietti @simonis @AlanBateman thank you for the suggestions! test/jdk/java/lang/String/RegionMatches.java was converted to testng and split into two @Tests, the latter now covers 8316879. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1735615353
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v2]
On Tue, 26 Sep 2023 12:38:46 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > add regression test @rgiulietti @simonis @AlanBateman thank you for the suggestions! test/jdk/java/lang/String/RegionMatches.java was converted to testng and split into two @Tests, the latter now covers 8316879. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1735615353
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v3]
> test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/String.java Co-authored-by: Raffaello Giulietti - Changes: - all: https://git.openjdk.org/jdk/pull/15906/files - new: https://git.openjdk.org/jdk/pull/15906/files/003c142e..7a505e0d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v2]
On Tue, 26 Sep 2023 12:38:46 GMT, Aleksei Voitylov wrote: >> test java.lang.String.RegionMatches1Tests fails on all platforms with >> -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by >> default. The fix is to return true immediately if len is negative, since for >> negative length this condition will never be satisfied. >> >> Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 >> and on ARM32. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > add regression test src/java.base/share/classes/java/lang/String.java line 2159: > 2157: return false; > 2158: } > 2159: // Any strings match if length < 0 Suggestion: // Any strings match if len < 0 - PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1337170256
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 [v2]
> test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: add regression test - Changes: - all: https://git.openjdk.org/jdk/pull/15906/files - new: https://git.openjdk.org/jdk/pull/15906/files/b95bf8f0..003c142e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=00-01 Stats: 26 lines in 1 file changed: 19 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
On Mon, 25 Sep 2023 15:52:12 GMT, Aleksei Voitylov wrote: > test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. As Volker said, a test should be added. Also please have a maintainer working in this area review it too. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1734864795
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
On Mon, 25 Sep 2023 15:52:12 GMT, Aleksei Voitylov wrote: > test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Marked as reviewed by m4ximumpi...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1643216765
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
On Mon, 25 Sep 2023 15:52:12 GMT, Aleksei Voitylov wrote: > test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Might be a good idea to add a simple regressions test. - PR Comment: https://git.openjdk.org/jdk/pull/15906#issuecomment-1734269182
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
On Mon, 25 Sep 2023 15:52:12 GMT, Aleksei Voitylov wrote: > test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Looks good. PS: I specifically like the "*Looks simple and harmless*" [comment on the PR of the original change](https://github.com/openjdk/jdk/pull/12528#pullrequestreview-1295839377) :) - Marked as reviewed by simonis (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1642774005
Re: RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
On Mon, 25 Sep 2023 15:52:12 GMT, Aleksei Voitylov wrote: > test java.lang.String.RegionMatches1Tests fails on all platforms with > -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by > default. The fix is to return true immediately if len is negative, since for > negative length this condition will never be satisfied. > > Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 > and on ARM32. Nice catch, but I don't think your current fix is correct: src/java.base/share/classes/java/lang/String.java line 2159: > 2157: return false; > 2158: } > 2159: // Any strings match if length < 0 I don't think this fix works, because the specification says: *The result is false if and only if at least one of the following is true:* - `toffset` is negative. - `ooffset` is negative. - `toffset+len` is greater than the length of this String object. - `ooffset+len` is greater than the length of the other argument. - There is some nonnegative integer `k` less than `len` such that: `this.charAt(toffset + k) != other.charAt(ooffset + k)` Now consider `"12345".regionMatches(10, "012345", 1, -3)` With the current implementation, the call returns `false` because `toffset + len` == `10 + (-3)` == 7 > 5 == `"12345".length()`. With your fix, the call will succeed because `len` == `-3` < `0`. - Changes requested by simonis (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15906#pullrequestreview-1642643067 PR Review Comment: https://git.openjdk.org/jdk/pull/15906#discussion_r1336170810
RFR: 8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163
test java.lang.String.RegionMatches1Tests fails on all platforms with -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by default. The fix is to return true immediately if len is negative, since for negative length this condition will never be satisfied. Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 and on ARM32. - Commit messages: - JDK-8316879 implementation Changes: https://git.openjdk.org/jdk/pull/15906/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15906&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8316879 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15906/head:pull/15906 PR: https://git.openjdk.org/jdk/pull/15906
