Re: RFR: 8359919: Minor java.util.concurrent doc improvements [v14]
On Sun, 22 Jun 2025 10:50:42 GMT, Doug Lea wrote: > Note that test j/u/c/tck/ testTimeUnit already includes > testTimedWait_IllegalMonitorException(). because it was already implied by > the spec. The change just makes this explicit. Does this require CSR? (If so, > I'd be happy to do it). I don't think it's strictly needed here, Joe Darcy is going to give an opinion today. - PR Comment: https://git.openjdk.org/jdk/pull/25880#issuecomment-2995219220
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 01:51:28 GMT, Naoto Sato wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix unicode escapes > > src/jdk.jlink/share/classes/jdk/tools/jimage/resources/jimage_de.properties > line 1: > >> 1: # > > Just wondering why this resource file has not been translated into > de/ja/zh_CN till now. Looks correct though. I am wondering the same thing. Perhaps because it is not intended for general public, it's not in the [JDK tools specifications](https://docs.oracle.com/en/java/javase/24/docs/specs/man/index.html). If that was intentionally left out, I don't see new reason to add them. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2160914804
Re: RFR: 8359830: Incorrect os.version reported on macOS Tahoe 26 (Beta)
On Mon, 23 Jun 2025 08:07:21 GMT, Jaikiran Pai wrote: > I think we are missing a test for this which might explain why tier1, tier2, > tier3 testing of this current change didn't notice that this area too needs a > change. It looks like an existing test `test/hotspot/jtreg/runtime/ErrorHandling/PrintVMInfoAtExitTest.java` might need an update to include a check for the OS version reported in the output. - PR Comment: https://git.openjdk.org/jdk/pull/25865#issuecomment-2995399123
Re: RFR: 8359830: Incorrect os.version reported on macOS Tahoe 26 (Beta)
On Wed, 18 Jun 2025 16:42:16 GMT, Kevin Rushforth wrote: >> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8359830? >> >> macOS operating system's newer version 26 (currently in Beta) is reported as >> a 16 by older versions of XCode. JDK internally uses the `NSProcessInfo` and >> `NSOperatingSystemVersion` APIs to identify the macOS version and set the >> `os.version` system property to that value. The current recommended version >> to build the JDK on macOS is XCode 15.4. The `NSOperatingSystemVersion` API >> on that version of XCode reports macOS version as 16 instead of 26. >> >> The commit in this PR updates the JDK code to handle this mismatch and set >> the `os.version` appropriately to 26. This fix is similar to what we did >> with macOS BigSur when the macOS version 10.16 was meant to mean 11 >> https://bugs.openjdk.org/browse/JDK-8253702. >> >> The existing `OsVersionTest` has been updated for some trivial clean up. >> Existing tests in tier1, tier2 and tier3 continue to pass with this change. >> If anyone has access to a macOS 26 Beta, I request them to build this change >> and run `tier1` tests to help verify that there aren't any failures. > > src/java.base/macosx/native/libjava/java_props_macosx.c line 242: > >> 240: const bool versionCompatEnabled = envVal != NULL && strncmp(envVal, >> "1", 1) == 0; >> 241: const bool requiresSpecialHandling = ((long) osVer.majorVersion == >> 10 && (long) osVer.minorVersion >= 16) >> 242: || ((long) osVer.majorVersion >> == 16 && (long) osVer.minorVersion >= 0); > > Since we know Apple jumped from 15 --> 26, would it be more future-proof to > change this test to something like this? > > > osVer.majorVersion >= 16 && osVer.majorVersion < 26 > > > This would allow us to work when macOS 27 comes out, in case it reports > itself as "17.0" if we don't update the version of Xcode before then. > > I can also see the argument for leaving it as you have done. And my guess is > that Apple will report macOS 27 as 27 regardless, so this probably doesn't > matter. Hello Kevin, > I can also see the argument for leaving it as you have done. I gave your suggestion some thought. I think it would be simpler (and less confusing) to leave that check in the current form and update it as and when needed depending on whether macOS does report future versions as two different values. - PR Review Comment: https://git.openjdk.org/jdk/pull/25865#discussion_r2160987245
Re: RFR: 8359919: Minor java.util.concurrent doc improvements [v15]
On Sun, 22 Jun 2025 12:54:29 GMT, Doug Lea wrote: >> This collects miscellaneous open issues that can be resolved with >> documentation updates; each indicated by adding JDK issue numbers > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 20 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8359919 > - Add JDK-8137156 > - Add JDK-6526284 doc update > - Add JDK-6625724 as docfix > - Add JDK-6714849 doc update > - Address review comments; add JDK-8172177 > - Fix ARFU type; add JDK-7176957 > - Address review comments; add JDK-6374942 > - Adding JDK-8333172 to doc improvements > - Add JDK-8195628 doc fix > - ... and 10 more: https://git.openjdk.org/jdk/compare/6f71ad56...944ddab2 Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25880#pullrequestreview-2948924091
Using jpackage 24 with WiX Toolset 5 or 6, how can I start the app automatically after installation is complete?
As subject. Using jpackage 24 with WiX Toolset 5 or 6, how can I start the app automatically after installation is complete? As title, is this possible? At the moment I generate my installer with this command: jpackage -i target --type exe --main-class org.dpsoftware.JavaFXStarter --main-jar FireflyLuciferin-jar-with-dependencies.jar --win-menu --win-menu-group Luciferin --copyright "Davide Perini" --name "Firefly Luciferin" --vendor DPsoftware --win-dir-chooser --win-shortcut --win-per-user-install --win-upgrade-uuid 33c82dc4-e0e0-11ea-87d0-0242ac130003 --app-version "0.0.5" --win-shortcut --win-shortcut-prompt --java-options "-XX:+UseZGC -XX:+UseStringDeduplication -Xms64m -Xmx1024m" How can I customize the installer to launch the application once the installation is finished? Thanks Davide
Integrated: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet
On Mon, 9 Jun 2025 20:42:43 GMT, Justin Lu wrote:
> Please review this PR which finishes Applet removal for the test:
> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>
> `testclasses.jar` is updated such that the two classes no longer extend
> Applet.
>
>
> $ javap fo\ o.class
> public class fo o {
> }
> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
> public class 手册 {
> }
>
>
> The bug description of
> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
> original `javap` output for those classes.
>
> Additionally, the security APIs that were marked for removal are also removed
> from this test as well.
This pull request has now been integrated.
Changeset: dfcea054
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/dfcea0547e7756a546fcf57855d99d46ecfb1925
Stats: 155 lines in 2 files changed: 17 ins; 115 del; 23 mod
8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on
Applet
Reviewed-by: jpai, lancea
-
PR: https://git.openjdk.org/jdk/pull/25703
Re: RFR: 8358729: jdk/internal/loader/URLClassPath/ClassnameCharTest.java depends on Applet [v7]
On Tue, 17 Jun 2025 21:47:49 GMT, Justin Lu wrote:
>> Please review this PR which finishes Applet removal for the test:
>> jdk/internal/loader/URLClassPath/ClassnameCharTest.java.
>>
>> `testclasses.jar` is updated such that the two classes no longer extend
>> Applet.
>>
>>
>> $ javap fo\ o.class
>> public class fo o {
>> }
>> $ javap æ$'\302\211'$'\302\213'å$'\302\206'$'\302\214'.class
>> public class 手册 {
>> }
>>
>>
>> The bug description of
>> [JDK-8358729](https://bugs.openjdk.org/browse/JDK-8358729) contains the
>> original `javap` output for those classes.
>>
>> Additionally, the security APIs that were marked for removal are also
>> removed from this test as well.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> SB -> String
Thank you for the reviews!
-
PR Comment: https://git.openjdk.org/jdk/pull/25703#issuecomment-2996708425
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Sun, 22 Jun 2025 01:13:26 GMT, simon wrote: > 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > @liach Can you review this PR ou assign someone that might be able to review? - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2995972169
Re: RFR: 8359830: Incorrect os.version reported on macOS Tahoe 26 (Beta)
On Wed, 18 Jun 2025 06:01:43 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the issue > noted in https://bugs.openjdk.org/browse/JDK-8359830? > > macOS operating system's newer version 26 (currently in Beta) is reported as > a 16 by older versions of XCode. JDK internally uses the `NSProcessInfo` and > `NSOperatingSystemVersion` APIs to identify the macOS version and set the > `os.version` system property to that value. The current recommended version > to build the JDK on macOS is XCode 15.4. The `NSOperatingSystemVersion` API > on that version of XCode reports macOS version as 16 instead of 26. > > The commit in this PR updates the JDK code to handle this mismatch and set > the `os.version` appropriately to 26. This fix is similar to what we did with > macOS BigSur when the macOS version 10.16 was meant to mean 11 > https://bugs.openjdk.org/browse/JDK-8253702. > > The existing `OsVersionTest` has been updated for some trivial clean up. > Existing tests in tier1, tier2 and tier3 continue to pass with this change. > If anyone has access to a macOS 26 Beta, I request them to build this change > and run `tier1` tests to help verify that there aren't any failures. Hello Alan, > Are there changes for os_bsd.cpp too? That's a good catch. I did not know that we have `os::get_summary_os_info` in that file which too deals with OS name and version. The native functions used in that implementation are `sysctl()` calls. I experimented on a macOS 26 (Beta) instance and those functions too report the "wrong" version (16) when the JDK is built using XCode 15.4. So this code needs to be addressed too. This `os::get_summary_os_info` gets used only through `os::print_summary_info` (in `os.cpp`), which gets called for VM crash info reporting as well as `java -XX:+UnlockDiagnosticVMOptions -XX:+PrintVMInfoAtExit ...`. I think we are missing a test for this which might explain why tier1, tier2, tier3 testing of this current change didn't notice that this area too needs a change. I'll update this PR to include this additional code change and introduce a new regression test to catch this issue. - PR Comment: https://git.openjdk.org/jdk/pull/25865#issuecomment-2995383253
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Mon, 23 Jun 2025 14:51:44 GMT, Chen Liang wrote: > FYI people don't usually review on weekends (you opened this PR in a weekend) > or holidays. This PR is sent to core-libs-dev list so people will eventually > see and review it. Great, @liach! Thanks for the information! 😄 - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2996843586
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 15:59:35 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix unicode escapes > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets_de.properties > line 229: > >> 227: doclet.search_in_documentation=In Dokumentation suchen >> 228: doclet.search_reset=Zurücksetzen >> 229: doclet.Member=Member > > Is `Member` translated? Should it be _“Mitglied”_? Good catch. Usually, we file these issues with the translation team and make the appropriate updates in the second L10n drop. I think this one is fine to be updated now as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2161839290
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Mon, 23 Jun 2025 14:50:24 GMT, Roger Riggs wrote: > The indentation fixes look ok. The reformatting of declarations seem > unnecessary and create lines that are longer than desirable to be able to do > side-by-side compares (100 char max). Generally, avoid just asking the IDE to > re-format the source, it leads to unnecessary changes. I'm going to undo the formatting changes where lines exceed 100 characters. Actually, I didn’t ask the IDE to reformat; I only suggested this formatting to improve readability. However, it can make comparisons in the diff view more difficult. @RogerRiggs Thanks for the review! 😃 - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2996839307
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov wrote:
> This PR contains changes for the 1st phase of the `LockingMode` flag
> obsoletion.
>
> The work is done by @fbredber, I have taken it over and am finishing it while
> he's on vacation.
>
> In the 1st phase one keeps the `LockingMode` variable in all places, but
> makes it non-settable from the command line. All the C1 and C2 code related
> to legacy locking will still be in place (but as dead code) and removed later
> (phase 2).
>
> Lightweight locking is the default locking from now on.
>
> Tested in tiers 1 - 7.
test/jdk/java/lang/Thread/virtual/Parking.java line 388:
> 386: @ParameterizedTest
> 387: @ValueSource(booleans = { true, false })
> 388: @DisabledIf("LockingMode#isLegacy")
Would you mind checking if the import DisabledIf can be removed from these
tests? I think we only used it to conditionally run when not legacy mode.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161946231
Integrated: 8359732: Make standard i/o encoding related system properties `StaticProperty`
On Tue, 17 Jun 2025 20:16:05 GMT, Naoto Sato wrote: > Refactored the internal handling of `stdin/out/err.encoding` to allow setting > them only via command-line options by converting them into `StaticProperty`. > This change prevents unexpected behavior caused by applications modifying > these properties at runtime using `System.setProperty()`. This pull request has now been integrated. Changeset: 9c3eaa49 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/9c3eaa49f7f8c6ade7319064566c0370e955f631 Stats: 51 lines in 7 files changed: 34 ins; 2 del; 15 mod 8359732: Make standard i/o encoding related system properties `StaticProperty` Reviewed-by: rriggs, alanb, vyazici - PR: https://git.openjdk.org/jdk/pull/25860
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 16:34:05 GMT, Alisen Chung wrote: >> I agree. I'd expect consistency here. > > What would be the correct translation of majorticks here? If "major tick" is translated to "Hauptteilstrich", then the plural would be `[SliderDemo.majorticks=]Hauptteilstriche` (which is a rather artificial word so the translation team might want to come up with a better translation for "major tick"). - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162131034
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 14:50:24 GMT, Roger Riggs wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: refactor code formatting to enforce 100 chars line length limit >> for improved readability > > The indentation fixes look ok. > The reformatting of declarations seem unnecessary and create lines that are > longer than desirable to be able to do side-by-side compares (100 char max). > Generally, avoid just asking the IDE to re-format the source, it leads to > unnecessary changes. @RogerRiggs I have just commited the changes to not exceed the 100 chars limit. - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2997252128
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: refactor code formatting to enforce 100 chars line length limit for improved readability - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/4310eaf9..3a7d0b5a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=00-01 Stats: 12 lines in 1 file changed: 8 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v4]
> This issue is responsible for updating the translations of all the > localize(able) resources in the JDK since the previous L10n drop. Alisen Chung has updated the pull request incrementally with one additional commit since the last revision: update to german translations - Changes: - all: https://git.openjdk.org/jdk/pull/25839/files - new: https://git.openjdk.org/jdk/pull/25839/files/66c34e7d..90bfd7bd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25839&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25839&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25839.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25839/head:pull/25839 PR: https://git.openjdk.org/jdk/pull/25839
Re: RFR: 8191963: Path.equals() and File.equals() return true for two different files on Windows
On Mon, 23 Jun 2025 11:09:30 GMT, Alan Bateman wrote: > WindowsPath.compareTo is where we ended up for JDK 7, it would be good to > validate this. I assume that you intend here validating correctness with respect to the NTFS case sensitivity settings - PR Comment: https://git.openjdk.org/jdk/pull/25788#issuecomment-2997183387
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:18:13 GMT, simon wrote:
>> 8360122: Refine formatting of Connection.java interface
>>
>> -
>> ### Progress
>> - [ ] Change must be properly reviewed (1 review required, with at least 1
>> [Reviewer](https://openjdk.org/bylaws#reviewer))
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>>
>>
>>
>> ### Reviewing
>> Using git
>>
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \
>> `$ git checkout pull/25925`
>>
>> Update a local copy of the PR: \
>> `$ git checkout pull/25925` \
>> `$ git pull https://git.openjdk.org/jdk.git pull/25925/head`
>>
>>
>> Using Skara CLI tools
>>
>> Checkout this PR locally: \
>> `$ git pr checkout 25925`
>>
>> View PR using the GUI difftool: \
>> `$ git pr show -t 25925`
>>
>>
>> Using diff file
>>
>> Download this PR as a diff file: \
>> > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff
>>
>>
>> Using Webrev
>>
>> [Link to Webrev
>> Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738)
>>
>
> simon has updated the pull request incrementally with one additional commit
> since the last revision:
>
> 8360122: refactor code formatting to enforce 100 chars line length limit
> for improved readability
src/java.sql/share/classes/java/sql/Connection.java line 1597:
> 1595: default boolean setShardingKeyIfValid(ShardingKey shardingKey,
> 1596: ShardingKey superShardingKey,
> 1597: int timeout) throws
> SQLException {
I haven't quite identified your preferred formatting style.
>From previous reformattings with multiple arguments, the exception was placed
>on a line by itself.
(Generally, we defer to the original/previous author's choice of line breaks
and formatting.
Choosing only to reformat to use a consistent style within each file/class or
package.)
The issue title and PR title should be a bit more general, its more than just
indentation cleanup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162126095
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Thu, 19 Jun 2025 17:39:38 GMT, Johannes Döbler wrote:
>> src/java.base/share/classes/sun/security/tools/keytool/resources/keytool_de.properties
>> line 183:
>>
>>> 181: size.bit.alg=%1$d-Bit-%2$s
>>> 182:
>>> Generating.full.keyAlgName.key.pair.and.self.signed.certificate.sigAlgName.with.a.validity.of.days.for=Generieren
>>> von {0}-Schlüsselpaar und selbstsigniertem Zertifikat ({1}) mit einer
>>> Gültigkeit von {2} Tagen\n\tfür: {3}
>>> 183:
>>> Generating.full.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.signerAlias.with.a.validity.of.days.for=Generieren
>>> von {0}-Schlüsselpaar und einem von <{2}> ausgestellten Zertifikat ({1})
>>> mit einer Gültigkeit von {3} Tagen\n\tfür: {4}
>>
>> It feels as if there's something missing after _‘einem’_.
>
> seems ok to me, einem von X ausgestellten Zertifikat ~ einem Zertifikat, das
> von X ausgestellt wurde
Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162395125
Re: RFR: 8357289: Break down the String constructor into smaller methods [v7]
On Mon, 26 May 2025 19:20:06 GMT, Shaojin Wen wrote:
>> Through JVM Option +PrintInlining, we found that String has a constructor
>> codeSize of 852, which is too large. This caused failed to inline.
>>
>> The following is the output information of PrintInlining:
>>
>> @ 9 java.lang.String:: (12 bytes) inline (hot)
>> !m @ 1 java.nio.charset.Charset::defaultCharset (52 bytes)
>> inline (hot)
>> ! @ 8 java.lang.String:: (852 bytes) failed to
>> inline: hot method too big
>>
>>
>> In Java code, the big method that cannot be inlined is the following
>> constructor
>>
>>
>> String(Charset charset, byte[] bytes, int offset, int length) {}
>>
>> The above String constructor is too large; break it down into smaller
>> methods with a codeSize under 325 to allow them to be inlined by the C2.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> from @liach
This looks like a good first step for bringing parity to string decoding and
encoding methods.
-
Marked as reviewed by liach (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25290#pullrequestreview-2951343600
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 18:38:00 GMT, simon wrote:
>> src/java.sql/share/classes/java/sql/Connection.java line 1597:
>>
>>> 1595: default boolean setShardingKeyIfValid(ShardingKey shardingKey,
>>> 1596: ShardingKey
>>> superShardingKey,
>>> 1597: int timeout) throws
>>> SQLException {
>>
>> I haven't quite identified your preferred formatting style.
>> From previous reformattings with multiple arguments, the exception was
>> placed on a line by itself.
>>
>> (Generally, we defer to the original/previous author's choice of line breaks
>> and formatting.
>> Choosing only to reformat to use a consistent style within each file/class
>> or package.)
>>
>> The issue title and PR title should be a bit more general, its more than
>> just indentation cleanup.
>
> @RogerRiggs My preferred formatting style is like this:
>
>
> default boolean setShardingKeyIfValid(ShardingKey shardingKey,
> ShardingKey superShardingKey,
> int timeout) throws SQLException {
>
>
> But as you said, there is previous reformattings that does not follow it. If
> you agree, I will fix them. What do you thing about it?
>
> About the issue title and PR title, we can make it more general like:
>
> "Improve Readability and Maintainability of Connection.java"
>
> What your suggestion on it?
I would backout the changes to signatures, fixing the bad indentation and leave
it at that.
Lance is one of the long time maintainers of the SQL classes and will want to
take a look.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162423668
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v4]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: revert reformatting method signatures - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/6d43550b..21696cf7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=02-03 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v4]
On Mon, 23 Jun 2025 23:09:13 GMT, simon wrote: >> 8360122: Refine formatting of Connection.java interface >> >> - >> ### Progress >> - [ ] Change must be properly reviewed (1 review required, with at least 1 >> [Reviewer](https://openjdk.org/bylaws#reviewer)) >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> >> >> >> ### Reviewing >> Using git >> >> Checkout this PR locally: \ >> `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ >> `$ git checkout pull/25925` >> >> Update a local copy of the PR: \ >> `$ git checkout pull/25925` \ >> `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` >> >> >> Using Skara CLI tools >> >> Checkout this PR locally: \ >> `$ git pr checkout 25925` >> >> View PR using the GUI difftool: \ >> `$ git pr show -t 25925` >> >> >> Using diff file >> >> Download this PR as a diff file: \ >> > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff >> >> >> Using Webrev >> >> [Link to Webrev >> Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) >> > > simon has updated the pull request incrementally with one additional commit > since the last revision: > > 8360122: revert reformatting method signatures The current version looks much better. - PR Review: https://git.openjdk.org/jdk/pull/25925#pullrequestreview-2951750084
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Mon, 23 Jun 2025 10:39:59 GMT, Anton Artemov wrote: >> test/hotspot/jtreg/runtime/Monitor/StressWrapper_TestRecursiveLocking_36M.java >> line 36: >> >>> 34: * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI >>> 35: * -Xint >>> 36: * -XX:LockingMode=0 >> >> I was wondering why these LockingMode=0 test cases were not setting >> `VerifyHeavyMonitors` instead, but I'm assuming the intent now is that we >> will only test that mode when it is set externally by the user (or in our >> case a particular test task definition)? >> >> I also realized we can only test heavy monitors in tests where we explicitly >> control the monitor creation places and hence can call the WB method to >> force inflation. That obviously reduces the test coverage for that mode >> quite significantly - but perhaps that will be handled if in the future we >> implicitly reenable forced inflation and do away with the WB usage. > > My understanding is that VerifyHeavyMonitors requires LockingMode = 0, see > line 1852 of arguments.cpp. So one has to set both at the same time, not one > instead of another. Now locking mode is hardcoded to lightweight, and there > is no way to use the incompatible `VerifyHeavyMonitors` option. My understanding was that `VerifyHeavyMonitors` was to be used as a replacement for `LockingMode=0` aka `UseHeavyMonitors`. But as Coleen has requested all `VerifyHeavyMonitors` testing be removed this is now a moot point. - PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2162866307
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov wrote:
> This PR contains changes for the 1st phase of the `LockingMode` flag
> obsoletion.
>
> The work is done by @fbredber, I have taken it over and am finishing it while
> he's on vacation.
>
> In the 1st phase one keeps the `LockingMode` variable in all places, but
> makes it non-settable from the command line. All the C1 and C2 code related
> to legacy locking will still be in place (but as dead code) and removed later
> (phase 2).
>
> Lightweight locking is the default locking from now on.
>
> Tested in tiers 1 - 7.
Changes are looking okay to me, but we have an issue with bug management that
needs to be resolved - and probably need a new bug and PR.
test/jtreg-ext/requires/VMProps.java line 424:
> 422: * Note: Lightweight locking does not support RTM (for now).
> 423: */
> 424: protected String vmRTMCompiler() {
[JDK-8358542](https://bugs.openjdk.org/browse/JDK-8358542) exists to remove
this so you would need to add that bug id to this PR. However, it seems the bug
management for this has gotten completely messed up so you may need to scrap
this PR and file a new bug and PR for this part.
-
PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2952042310
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2162873188
Re: RFR: 8191963: Path.equals() and File.equals() return true for two different files on Windows
On Mon, 16 Jun 2025 22:49:00 GMT, Xueming Shen wrote: > Just wondering how Windows implementation really behaves for its > 'case-insensitive-comparing" for "\u0131 vs "I". I would assume they actually > are 2 'distinguished" files? I think it depends on the locale. Also it is possible to configure NTFS, on a per directory basis, to disable the directory the case sensitivity. WindowsPath.compareTo is where we ended up for JDK 7, it would be good to validate this. - PR Comment: https://git.openjdk.org/jdk/pull/25788#issuecomment-2995994694
RFR: 8359437: Make users and test suite not able to set LockingMode flag
This PR contains changes for the 1st phase of the `LockingMode` flag obsoletion. The work is done by @fbredber, I have taken it over and am finishing it while he's on vacation. In the 1st phase one keeps the `LockingMode` variable in all places, but makes it non-settable from the command line. All the C1 and C2 code related to legacy locking will still be in place (but as dead code) and removed later (phase 2). Lightweight locking is the default locking from now on. Tested in tiers 1 - 7. - Commit messages: - Merge remote-tracking branch 'origin/master' into 8359437_locking_mode_obsoletion - Merge remote-tracking branch 'origin/master' into 8359437_locking_mode_obsoletion - 8359437: Addressed reviewers' comments - Merge remote-tracking branch 'origin/master' into 8359437_locking_mode_obsoletion - Merge remote-tracking branch 'origin/master' into 8359437_locking_mode_obsoletion - Merge branch 'master' into _remove_locking_mode_fix_tests - Update after pre-review - First try. Changes: https://git.openjdk.org/jdk/pull/25847/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25847&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8359437 Stats: 1122 lines in 33 files changed: 40 ins; 992 del; 90 mod Patch: https://git.openjdk.org/jdk/pull/25847.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25847/head:pull/25847 PR: https://git.openjdk.org/jdk/pull/25847
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v3]
On Wed, 18 Jun 2025 18:48:04 GMT, Damon Nguyen wrote: >> src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources_de.java >> line 2: >> >>> 1: /* >>> 2: * Copyright (c) 2001, 2023, Oracle and/or its affiliates. All rights >>> reserved. >> >> Looks wrong but is correct. File had its copyright year updated in >> https://bugs.openjdk.org/browse/JDK-8345800, but the original is still 2023. >> Translation team is re-syncing it to match the original year. > > Is there any way to prevent this or do we just change it back when we do a > drop and explain why each time? If this is committed, then the years will be synced and we won't have to change it back. This would be consistent with the way we receive l10n translations which sync the copyright years of localized files to the original. If we want to deviate from this then we could file a request asking them to not update copyright years and we could update them ourselves. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2161805058
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Wed, 18 Jun 2025 18:23:58 GMT, Coleen Phillimore wrote:
>> This PR contains changes for the 1st phase of the `LockingMode` flag
>> obsoletion.
>>
>> The work is done by @fbredber, I have taken it over and am finishing it
>> while he's on vacation.
>>
>> In the 1st phase one keeps the `LockingMode` variable in all places, but
>> makes it non-settable from the command line. All the C1 and C2 code related
>> to legacy locking will still be in place (but as dead code) and removed
>> later (phase 2).
>>
>> Lightweight locking is the default locking from now on.
>>
>> Tested in tiers 1 - 7.
>
> test/hotspot/jtreg/runtime/Monitor/TestRecursiveLocking.java line 125:
>
>> 123: public class TestRecursiveLocking {
>> 124: static final WhiteBox WB = WhiteBox.getWhiteBox();
>> 125: static final boolean flagHeavyMonitors =
>> WB.getBooleanVMFlag("VerifyHeavyMonitors");
>
> I think you should take out the VerifyHeavyMonitors cases. @fbredber
> originally had that flag turn on a the reintroduced UseHeavyMonitors option
> but the UseHeavyMonitors option doesn't actually do that with this change. I
> don't think this test will pass with -XX:+VerifyHeavyMonitors.
> If we reintroduce UseHeavyMonitors, save this diff and fix this test then.
> Right now it's not correct.
Removed in the latest commit.
> test/hotspot/jtreg/runtime/lockStack/TestLockStackCapacity.java line 42:
>
>> 40: public class TestLockStackCapacity {
>> 41: static final WhiteBox WB = WhiteBox.getWhiteBox();
>> 42: static final boolean flagHeavyMonitors =
>> WB.getBooleanVMFlag("VerifyHeavyMonitors");
>
> I think this should also not have cases for VerifyHeavyMonitors. We can add
> back tests if we want UseHeavyMonitors. As of now, removing the Legacy
> locking code will remove code that reaches the VerifyHeavyMonitors branches.
Removed in the latest commit.
> test/jtreg-ext/requires/VMProps.java line 424:
>
>> 422: * Note: Lightweight locking does not support RTM (for now).
>> 423: */
>> 424: protected String vmRTMCompiler() {
>
> There's an issue to remove this function since it's now unused.
Removed in the latest commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282585
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282345
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161282101
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Wed, 18 Jun 2025 07:52:02 GMT, David Holmes wrote: >> This PR contains changes for the 1st phase of the `LockingMode` flag >> obsoletion. >> >> The work is done by @fbredber, I have taken it over and am finishing it >> while he's on vacation. >> >> In the 1st phase one keeps the `LockingMode` variable in all places, but >> makes it non-settable from the command line. All the C1 and C2 code related >> to legacy locking will still be in place (but as dead code) and removed >> later (phase 2). >> >> Lightweight locking is the default locking from now on. >> >> Tested in tiers 1 - 7. > > test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java line 1: > >> 1: /* > > This seems to remove significant test coverage. can we not adapt the tests to > not rely on logging warnings that will no longer be present? The premise of this test is now invalid. We could write a fresh new test if we'd like to see what happens with UseHeavyMonitors, and/or retrieve this from git history. - PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155250065
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov wrote:
> This PR contains changes for the 1st phase of the `LockingMode` flag
> obsoletion.
>
> The work is done by @fbredber, I have taken it over and am finishing it while
> he's on vacation.
>
> In the 1st phase one keeps the `LockingMode` variable in all places, but
> makes it non-settable from the command line. All the C1 and C2 code related
> to legacy locking will still be in place (but as dead code) and removed later
> (phase 2).
>
> Lightweight locking is the default locking from now on.
>
> Tested in tiers 1 - 7.
I have a request for some more deieting.
This looks great. Thank you!
test/hotspot/jtreg/runtime/Monitor/TestRecursiveLocking.java line 125:
> 123: public class TestRecursiveLocking {
> 124: static final WhiteBox WB = WhiteBox.getWhiteBox();
> 125: static final boolean flagHeavyMonitors =
> WB.getBooleanVMFlag("VerifyHeavyMonitors");
I think you should take out the VerifyHeavyMonitors cases. @fbredber
originally had that flag turn on a the reintroduced UseHeavyMonitors option but
the UseHeavyMonitors option doesn't actually do that with this change. I don't
think this test will pass with -XX:+VerifyHeavyMonitors.
If we reintroduce UseHeavyMonitors, save this diff and fix this test then.
Right now it's not correct.
test/hotspot/jtreg/runtime/lockStack/TestLockStackCapacity.java line 42:
> 40: public class TestLockStackCapacity {
> 41: static final WhiteBox WB = WhiteBox.getWhiteBox();
> 42: static final boolean flagHeavyMonitors =
> WB.getBooleanVMFlag("VerifyHeavyMonitors");
I think this should also not have cases for VerifyHeavyMonitors. We can add
back tests if we want UseHeavyMonitors. As of now, removing the Legacy locking
code will remove code that reaches the VerifyHeavyMonitors branches.
test/jtreg-ext/requires/VMProps.java line 424:
> 422: * Note: Lightweight locking does not support RTM (for now).
> 423: */
> 424: protected String vmRTMCompiler() {
There's an issue to remove this function since it's now unused.
-
Changes requested by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2940262141
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2949835857
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155234150
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155246651
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2155256340
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Wed, 18 Jun 2025 07:37:31 GMT, David Holmes wrote:
>> This PR contains changes for the 1st phase of the `LockingMode` flag
>> obsoletion.
>>
>> The work is done by @fbredber, I have taken it over and am finishing it
>> while he's on vacation.
>>
>> In the 1st phase one keeps the `LockingMode` variable in all places, but
>> makes it non-settable from the command line. All the C1 and C2 code related
>> to legacy locking will still be in place (but as dead code) and removed
>> later (phase 2).
>>
>> Lightweight locking is the default locking from now on.
>>
>> Tested in tiers 1 - 7.
>
> src/hotspot/share/runtime/arguments.cpp line 1839:
>
>> 1837: #ifndef _LP64
>> 1838: if (LockingMode == LM_LEGACY) {
>> 1839: LockingMode = LM_LIGHTWEIGHT;
>
> If we have prevented the locking mode from being set then surely we can never
> encounter this case?
Looks like yes, this whole check then can be removed. Addressed in the latest
commit.
> src/hotspot/share/utilities/globalDefinitions.cpp line 59:
>
>> 57: uint64_t OopEncodingHeapMax = 0;
>> 58:
>> 59: int LockingMode = LM_LIGHTWEIGHT;
>
> const ?
This can be done provided that one removes assignment on line 3763 in
arguments.cpp. That assignment looks redundant as LockingMode is always
LM_LIGHTWEIGHT from now on.
> src/hotspot/share/utilities/globalDefinitions.hpp line 1012:
>
>> 1010: };
>> 1011:
>> 1012: extern int LockingMode;
>
> const ?
This can be done provided that one removes assignment on line 3763 in
arguments.cpp. That assignment looks redundant as LockingMode is always
LM_LIGHTWEIGHT from now on.
> test/hotspot/jtreg/runtime/Monitor/StressWrapper_TestRecursiveLocking_36M.java
> line 36:
>
>> 34: * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
>> 35: * -Xint
>> 36: * -XX:LockingMode=0
>
> I was wondering why these LockingMode=0 test cases were not setting
> `VerifyHeavyMonitors` instead, but I'm assuming the intent now is that we
> will only test that mode when it is set externally by the user (or in our
> case a particular test task definition)?
>
> I also realized we can only test heavy monitors in tests where we explicitly
> control the monitor creation places and hence can call the WB method to force
> inflation. That obviously reduces the test coverage for that mode quite
> significantly - but perhaps that will be handled if in the future we
> implicitly reenable forced inflation and do away with the WB usage.
My understanding is that VerifyHeavyMonitors requires LockingMode = 0, see line
1852 of arguments.cpp. So one has to set both at the same time, not one instead
of another. Now locking mode is hardcoded to lightweight, and there is no way
to use the incompatible `VerifyHeavyMonitors` option.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161284370
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161120254
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161119703
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2161284143
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov wrote:
> This PR contains changes for the 1st phase of the `LockingMode` flag
> obsoletion.
>
> The work is done by @fbredber, I have taken it over and am finishing it while
> he's on vacation.
>
> In the 1st phase one keeps the `LockingMode` variable in all places, but
> makes it non-settable from the command line. All the C1 and C2 code related
> to legacy locking will still be in place (but as dead code) and removed later
> (phase 2).
>
> Lightweight locking is the default locking from now on.
>
> Tested in tiers 1 - 7.
I've taken an initial pass through. Initially I misunderstood the strategy with
heavy monitors - see comments below.
src/hotspot/share/runtime/arguments.cpp line 1839:
> 1837: #ifndef _LP64
> 1838: if (LockingMode == LM_LEGACY) {
> 1839: LockingMode = LM_LIGHTWEIGHT;
If we have prevented the locking mode from being set then surely we can never
encounter this case?
src/hotspot/share/utilities/globalDefinitions.cpp line 59:
> 57: uint64_t OopEncodingHeapMax = 0;
> 58:
> 59: int LockingMode = LM_LIGHTWEIGHT;
const ?
src/hotspot/share/utilities/globalDefinitions.hpp line 1012:
> 1010: };
> 1011:
> 1012: extern int LockingMode;
const ?
test/hotspot/jtreg/runtime/Monitor/StressWrapper_TestRecursiveLocking_36M.java
line 36:
> 34: * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
> 35: * -Xint
> 36: * -XX:LockingMode=0
I was wondering why these LockingMode=0 test cases were not setting
`VerifyHeavyMonitors` instead, but I'm assuming the intent now is that we will
only test that mode when it is set externally by the user (or in our case a
particular test task definition)?
I also realized we can only test heavy monitors in tests where we explicitly
control the monitor creation places and hence can call the WB method to force
inflation. That obviously reduces the test coverage for that mode quite
significantly - but perhaps that will be handled if in the future we implicitly
reenable forced inflation and do away with the WB usage.
test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java line 1:
> 1: /*
This seems to remove significant test coverage. can we not adapt the tests to
not rely on logging warnings that will no longer be present?
-
PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2938076106
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153873165
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153924585
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153924946
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153884907
PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2153911036
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Sun, 22 Jun 2025 01:13:26 GMT, simon wrote: > 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > FYI people don't usually review on weekends (you opened this PR in a weekend) or holidays. This PR is sent to core-libs-dev list so people will eventually see and review it. - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2996805595
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Sun, 22 Jun 2025 01:13:26 GMT, simon wrote: > 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > The indentation fixes look ok. The reformatting of declarations seem unnecessary and create lines that are longer than desirable to be able to do side-by-side compares (100 char max). Generally, avoid just asking the IDE to re-format the source, it leads to unnecessary changes. - PR Review: https://git.openjdk.org/jdk/pull/25925#pullrequestreview-2950430779
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 15:52:15 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix unicode escapes > > src/jdk.jartool/share/classes/sun/security/tools/jarsigner/resources/jarsigner_de.properties > line 220: > >> 218: >> entry.1.is.signed.in.jarfile.but.is.not.signed.in.jarinputstream=Eintrag %s >> ist in JarFile, aber nicht in JarInputStream signiert >> 219: >> entry.1.is.signed.in.jarinputstream.but.is.not.signed.in.jarfile=Eintrag %s >> ist in JarInputStream, aber nicht in JarFile signiert >> 220: >> jar.contains.internal.inconsistencies.result.in.different.contents.via.jarfile.and.jarinputstream=Diese >> JAR-Datei enthält interne Inkonsistenzen, die zu anderem Inhalt beim Lesen >> über JarFile als beim Lesen über JarInputStream führen können: > > Is it expected to have “JAR_-Datei_”? It was removed from `.verified` and > `.signed` strings. I think it is because the English version of the properties file uses "jar" for `.verified` and `.signed` strings, but `jar.contains.internal.inconsistencies.result.in.different.contents.via.jarfile.and.jarinputstream` uses "JAR". - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2161832785
Re: Using jpackage 24 with WiX Toolset 5 or 6, how can I start the app automatically after installation is complete?
Hi Davide, There is no such jpackage option that configures the installer to launch the app automatically after installation. You can, however, customize wix sources jpackage uses to build the installer accordingly. Here is how you can do it: 1. Use main.wxs file from [1] as the baseline for your custom main.wxs file. 2. Adjust it following steps from the tutorial at [2]. The tutorial applies to Wix3, but it should also work with Wix5. 3. Place your custom main.wxs file in the resource directory (use --resource-dir option of jpackage) to make jpackage use the custom main.wxs file instead of the default one. Your question is a good candidate for a feature request, but until jpackage supports auto-launch of the installed app, manual wix sources modification is the only available option. [1] https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/resources/main.wxs [2] https://docs.firegiant.com/wix3/howtos/ui_and_localization/run_program_after_install/ - Alexey On 6/23/2025 10:26 AM, Davide Perini wrote: As subject. Using jpackage 24 with WiX Toolset 5 or 6, how can I start the app automatically after installation is complete? As title, is this possible? At the moment I generate my installer with this command: jpackage -i target --type exe --main-class org.dpsoftware.JavaFXStarter --main-jar FireflyLuciferin-jar-with-dependencies.jar --win-menu --win-menu-group Luciferin --copyright "Davide Perini" --name "Firefly Luciferin" --vendor DPsoftware --win-dir-chooser --win-shortcut --win-per-user-install --win-upgrade-uuid 33c82dc4-e0e0-11ea-87d0-0242ac130003 --app-version "0.0.5" --win-shortcut --win-shortcut-prompt --java-options "-XX:+UseZGC -XX:+UseStringDeduplication -Xms64m -Xmx1024m" How can I customize the installer to launch the application once the installation is finished? Thanks Davide
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 07:31:34 GMT, Henry Jen wrote: >> src/jdk.jlink/share/classes/jdk/tools/jimage/resources/jimage_de.properties >> line 1: >> >>> 1: # >> >> Just wondering why this resource file has not been translated into >> de/ja/zh_CN till now. Looks correct though. > > I am wondering the same thing. Perhaps because it is not intended for general > public, it's not in the [JDK tools > specifications](https://docs.oracle.com/en/java/javase/24/docs/specs/man/index.html). > If that was intentionally left out, I don't see new reason to add them. This file wasn't originally in the tbom file when we first restarted translations and was picked up by our getChanges tool because of a recent update to the file some time after the previous drop, so it hasn't been translated until this drop. Maybe the previous team just forgot to translate this file? - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162024606
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 14:48:13 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix unicode escapes > > src/demo/share/jfc/SwingSet2/resources/swingset_de.properties line 462: > >> 460: SliderDemo.majorticksdescription=Ein Schieberegler mit Hauptteilstrichen >> 461: SliderDemo.ticks=Feinteilungen, Teilungen zum Einrasten und Labels >> 462: SliderDemo.minorticks=Feinteilungen > > The following description uses different words now: > > Feinteilungen -> Teilstrichen > Teilungen -> Teilstrichen Just to clarify, does this mean SliderDemo.ticks=Teilstrichen, Teilstrichen zum Einrasten und Labels SliderDemo.minorticks=Teilstrichen is correct? - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162030543
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 20:37:43 GMT, Phil Race wrote: >> src/demo/share/jfc/SwingSet2/resources/swingset_de.properties line 460: >> >>> 458: SliderDemo.a_plain_slider=Ein einfacher Schieberegler >>> 459: SliderDemo.majorticks=Grobteilungen >>> 460: SliderDemo.majorticksdescription=Ein Schieberegler mit >>> Hauptteilstrichen >> >> Should the translation of `SliderDemo.majorticks` also be updated? > > I agree. I'd expect consistency here. What would be the correct translation of majorticks here? - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162027420
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 16:32:21 GMT, Alisen Chung wrote: >> I am wondering the same thing. Perhaps because it is not intended for >> general public, it's not in the [JDK tools >> specifications](https://docs.oracle.com/en/java/javase/24/docs/specs/man/index.html). >> If that was intentionally left out, I don't see new reason to add them. > > This file wasn't originally in the tbom file when we first restarted > translations and was picked up by our getChanges tool because of a recent > update to the file some time after the previous drop, so it hasn't been > translated until this drop. Maybe the previous team just forgot to translate > this file? Thanks. I guess this was simply an overlook in the first place, as the process was manual back then. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162038459
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:35:50 GMT, Roger Riggs wrote:
>> simon has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> 8360122: refactor code formatting to enforce 100 chars line length limit
>> for improved readability
>
> src/java.sql/share/classes/java/sql/Connection.java line 1597:
>
>> 1595: default boolean setShardingKeyIfValid(ShardingKey shardingKey,
>> 1596: ShardingKey superShardingKey,
>> 1597: int timeout) throws
>> SQLException {
>
> I haven't quite identified your preferred formatting style.
> From previous reformattings with multiple arguments, the exception was placed
> on a line by itself.
>
> (Generally, we defer to the original/previous author's choice of line breaks
> and formatting.
> Choosing only to reformat to use a consistent style within each file/class or
> package.)
>
> The issue title and PR title should be a bit more general, its more than just
> indentation cleanup.
@RogerRiggs My preferred formatting style is like this:
default boolean setShardingKeyIfValid(ShardingKey shardingKey,
ShardingKey superShardingKey,
int timeout) throws SQLException {
But as you said, there is previous reformattings that does not follow it. If
you agree, I will fix them. What do you thing about it?
About the issue title and PR title, we can make it more general like:
"Improve Readability and Maintainability of Connection.java"
What your suggestion on it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162305006
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 14:55:08 GMT, Justin Lu wrote: >> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/resources/jarsigner_de.properties >> line 220: >> >>> 218: >>> entry.1.is.signed.in.jarfile.but.is.not.signed.in.jarinputstream=Eintrag %s >>> ist in JarFile, aber nicht in JarInputStream signiert >>> 219: >>> entry.1.is.signed.in.jarinputstream.but.is.not.signed.in.jarfile=Eintrag %s >>> ist in JarInputStream, aber nicht in JarFile signiert >>> 220: >>> jar.contains.internal.inconsistencies.result.in.different.contents.via.jarfile.and.jarinputstream=Diese >>> JAR-Datei enthält interne Inkonsistenzen, die zu anderem Inhalt beim Lesen >>> über JarFile als beim Lesen über JarInputStream führen können: >> >> Is it expected to have “JAR_-Datei_”? It was removed from `.verified` and >> `.signed` strings. > > I think it is because the English version of the properties file uses "jar" > for `.verified` and `.signed` strings, but > `jar.contains.internal.inconsistencies.result.in.different.contents.via.jarfile.and.jarinputstream` > uses "JAR". I see the entries for jar.signed.=jar signed. jar.verified.=jar verified. don't have the word *“file”*, whereas `…via.jarfile.and.jarinputstream` has it: “This JAR *file*…” - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162318454
Re: RFR: 8358533: Improve performance of java.io.Reader.readAllLines
On Thu, 19 Jun 2025 11:08:32 GMT, Markus KARG wrote: >> Replaces the implementation `readAllCharsAsString().lines().toList()` with >> reading into a temporary `char` array which is then processed to detect line >> terminators and copy non-terminating characters into strings which are added >> to the list. > > src/java.base/share/classes/java/io/Reader.java line 508: > >> 506: } >> 507: >> 508: return lines; > > Do we really want to return a mutable `ArrayList` here? In earlier > discussions about this very API I was told that it deliberately returns > `String` instead of `CharSequence` due to *intended* immutability, even if > that potentially implied slower performance. Following this logic, it would > be just straightforward to `return Collections.unmodifiableList(lines);` > here. 🤔 Right, the specification here requires an unmodifiable List, so an unmodifiable wrapper or a list from `List.copyOf()` is appropriate. - PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r2162555958
Re: RFR: 8360045: StringTokenizer.hasMoreTokens() throws NPE after nextToken(null)
On Mon, 23 Jun 2025 20:14:36 GMT, Naoto Sato wrote:
> Fixing the side-effect caused by calling `StringTokenizer.nextToken(null)`,
> where the delimiter is set to `null` even if the method throws an NPE.
Note that as mentioned in the bug discussion, the same also occurs when doing
`new StringTokenizer("some string", (String) null)`.
-
PR Comment: https://git.openjdk.org/jdk/pull/25942#issuecomment-2997985004
Re: RFR: 8354872: Clarify java.lang.Process resource cleanup
On Wed, 18 Jun 2025 20:16:18 GMT, Roger Riggs wrote: > Improve the documentation of Process use of system resources. > > Describe the implementation closing streams when no longer referenced. > Clarify the interactions between inputStream and inputReader and errorStream > and errorReader. > Add advice and example using try-with-resources to open and close streams. > Recommend closing streams when no longer in use. The addendum on resource management looks very useful, thanks! 💯 Shall we also 1. Create a dedicated section on resource management 2. Document the effect of `destroy()` and `destroyForcibly()` (and `onExit()`?) on releasing resources - PR Review: https://git.openjdk.org/jdk/pull/25884#pullrequestreview-2951417760
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:18:13 GMT, simon wrote: >> 8360122: Refine formatting of Connection.java interface >> >> - >> ### Progress >> - [ ] Change must be properly reviewed (1 review required, with at least 1 >> [Reviewer](https://openjdk.org/bylaws#reviewer)) >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> >> >> >> ### Reviewing >> Using git >> >> Checkout this PR locally: \ >> `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ >> `$ git checkout pull/25925` >> >> Update a local copy of the PR: \ >> `$ git checkout pull/25925` \ >> `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` >> >> >> Using Skara CLI tools >> >> Checkout this PR locally: \ >> `$ git pr checkout 25925` >> >> View PR using the GUI difftool: \ >> `$ git pr show -t 25925` >> >> >> Using diff file >> >> Download this PR as a diff file: \ >> > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff >> >> >> Using Webrev >> >> [Link to Webrev >> Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) >> > > simon has updated the pull request incrementally with one additional commit > since the last revision: > > 8360122: refactor code formatting to enforce 100 chars line length limit > for improved readability I might not have time this week to look at this as I have some other commitments. I do think we will want to narrow the changes being proposed - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2997341500
Re: RFR: 8358627: tools/sincechecker/modules/java.base/JavaBaseCheckSince.java fails with JDK 26
On Tue, 17 Jun 2025 15:22:24 GMT, Nizar Benalla wrote: > Once https://bugs.openjdk.org/browse/JDK-8358769 is resolved, > JavaBaseCheckSince no longer needs to be problemlisted. Now that https://github.com/openjdk/jdk/pull/25854 has been integrated, the tests no longer need to be problemlisted. - PR Comment: https://git.openjdk.org/jdk/pull/25855#issuecomment-2998140113
RFR: 8360303: Remove two unused invoke files
sun.invoke.empty.Empty and java.lang.invoke.InvokeDynamic are useless remnants - Empty can be replaced by java.lang.Void, and InvokeDynamic was previously used as a stub for javac to compile signature polymorphic methods. They should be removed as a cleanup. Testing: jdk/java/lang/invoke - Commit messages: - Remove two useless invoke files Changes: https://git.openjdk.org/jdk/pull/25944/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25944&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8360303 Stats: 79 lines in 4 files changed: 0 ins; 77 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25944/head:pull/25944 PR: https://git.openjdk.org/jdk/pull/25944
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v3]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: revert reformatting method signatures - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/3a7d0b5a..6d43550b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=01-02 Stats: 29 lines in 1 file changed: 8 ins; 5 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v4]
On Mon, 23 Jun 2025 16:44:23 GMT, Alisen Chung wrote:
>> This issue is responsible for updating the translations of all the
>> localize(able) resources in the JDK since the previous L10n drop.
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update to german translations
src/jdk.compiler/share/classes/com/sun/tools/javac/resources/launcher_de.properties
line 103:
> 101:
> 102: # 0: string
> 103: launcher.err.cant.find.main.method=main(String[])- oder main()-Methode
> nicht gefunden in Klasse: {0}
Suggestion:
launcher.err.cant.find.main.method=Konnte keine main(String[])- oder
main()-Methode in der Klasse: {0} finden.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2163006433
Re: RFR: 8360045: StringTokenizer.hasMoreTokens() throws NPE after nextToken(null)
On Mon, 23 Jun 2025 20:14:36 GMT, Naoto Sato wrote: > Fixing the side-effect caused by calling `StringTokenizer.nextToken(null)`, > where the delimiter is set to `null` even if the method throws an NPE. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25942#pullrequestreview-2952322739
Re: RFR: 8294226: Document missing UnsupportedTemporalTypeException
On Mon, 16 Jun 2025 16:36:43 GMT, Gautham Krishnan wrote: > Some methods in the java.time.chrono interfaces—ChronoLocalDate, > ChronoLocalDateTime, and ChronoZonedDateTime—override methods from the > java.time.temporal.Temporal interface that are documented to throw > UnsupportedTemporalTypeException when given unsupported fields or units. > > These overridden methods include: > > with(TemporalField, long) > > plus(long, TemporalUnit) > > minus(long, TemporalUnit) > > However, their Javadoc in the chrono interfaces does not mention the > possibility of this exception, resulting in incomplete or inconsistent > documentation. This contrasts with the parent Temporal interface, which > explicitly documents the exception. @gauthamkrishnanibm Your change (at version 15558c58513ff2168ae57a67d36ea12492534ffd) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/25836#issuecomment-2998753203
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 19:04:22 GMT, Alexey Ivanov wrote: >> If "major tick" is translated to "Hauptteilstrich", then the plural would be >> `[SliderDemo.majorticks=]Hauptteilstriche` (which is a rather artificial >> word so the translation team might want to come up with a better translation >> for "major tick"). > > Microsoft uses *Hauptteilstrich*, see [MajorTickMark > Klasse](https://learn.microsoft.com/de-de/dotnet/api/documentformat.openxml.drawing.charts.majortickmark?view=openxml-3.0.1), > so *Hauptteilstrich* (singular Nominative) seems correct. Ja, "Hauptteilstrich" and "Hauptteilstriche" (pl) are correct. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162975420
Re: RFR: 8360045: StringTokenizer.hasMoreTokens() throws NPE after nextToken(null)
On Mon, 23 Jun 2025 20:14:36 GMT, Naoto Sato wrote: > Fixing the side-effect caused by calling `StringTokenizer.nextToken(null)`, > where the delimiter is set to `null` even if the method throws an NPE. Indeed, looking at the spec, we would anticipate an exception means no change has been committed to a StringTokenizer. - Marked as reviewed by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25942#pullrequestreview-2951520968
Re: RFR: 8359732: Make standard i/o encoding related system properties `StaticProperty` [v4]
On Fri, 20 Jun 2025 18:44:47 GMT, Naoto Sato wrote: >> Refactored the internal handling of `stdin/out/err.encoding` to allow >> setting them only via command-line options by converting them into >> `StaticProperty`. This change prevents unexpected behavior caused by >> applications modifying these properties at runtime using >> `System.setProperty()`. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments. Limits the usage of StaticProperty within > java.base Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/25860#issuecomment-2997091425
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 16:36:00 GMT, Alisen Chung wrote: >> src/demo/share/jfc/SwingSet2/resources/swingset_de.properties line 462: >> >>> 460: SliderDemo.majorticksdescription=Ein Schieberegler mit >>> Hauptteilstrichen >>> 461: SliderDemo.ticks=Feinteilungen, Teilungen zum Einrasten und Labels >>> 462: SliderDemo.minorticks=Feinteilungen >> >> The following description uses different words now: >> >> Feinteilungen -> Teilstrichen >> Teilungen -> Teilstrichen > > Just to clarify, does this mean > > SliderDemo.ticks=Teilstrichen, Teilstrichen zum Einrasten und Labels > SliderDemo.minorticks=Teilstrichen > > is correct? Hm… I don't know. You should clarify with the translation team. They've changed the translation for the major ticks from *Grobteilungen* to *Hauptteilstriche*. Yet minor ticks remain *Feinteilungen*, which doesn't look right because the main word is different. (*Teilung* doesn't seem to be the right word, at least dictionaries translate it as “division”.) *Teilstrich* means *scale line*, the marks on a ruler, so it fits. Microsoft documentation for [MinorTickMark Klasse](https://learn.microsoft.com/de-de/dotnet/api/documentformat.openxml.drawing.charts.minortickmark?view=openxml-3.0.1) uses *Kleinere Teilstriche*, yet I don't understand why there's ‘-e’ ending (plural?). SliderDemo.ticks=Teilstrichen, Teilstrichen zum Einrasten und Labels This one looks particularly incorrect; “einrasten“ is a verb with a separable prefix that means “to lock”, and I can't find a usage as if it's a noun. The English phrase is “Minor Ticks, Snap-to-ticks and Labels”, and I don't know how to correctly translate it to German. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162394521
Re: RFR: 8359919: Minor java.util.concurrent doc improvements [v14]
On Mon, 23 Jun 2025 07:14:45 GMT, Alan Bateman wrote: > > Note that test j/u/c/tck/ testTimeUnit already includes > > testTimedWait_IllegalMonitorException(). because it was already implied by > > the spec. The change just makes this explicit. Does this require CSR? (If > > so, I'd be happy to do it). > > I don't think it's strictly needed here, Joe Darcy is going to give an > opinion today. Okay -- I've taken a quick look over the proposed changes. While most of the changes are clearly informative (and thus don't need a CSR), I think some of them would fall over the line of what needs a CSR (some changes in TimeUnit.java, RunnableFuture.java, Flow.java). Further work might show those other changes and already implied by other parts of the spec, but I think it is less effort overall just to run a quick CSR for this change. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/25880#issuecomment-2997849940
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 19:54:24 GMT, Roger Riggs wrote:
>> @RogerRiggs My preferred formatting style is like this:
>>
>>
>> default boolean setShardingKeyIfValid(ShardingKey shardingKey,
>> ShardingKey superShardingKey,
>> int timeout) throws SQLException {
>>
>>
>> But as you said, there is previous reformattings that does not follow it. If
>> you agree, I will fix them. What do you thing about it?
>>
>> About the issue title and PR title, we can make it more general like:
>>
>> "Improve Readability and Maintainability of Connection.java"
>>
>> What your suggestion on it?
>
> I would backout the changes to signatures, fixing the bad indentation and
> leave it at that.
> Lance is one of the long time maintainers of the SQL classes and will want to
> take a look.
Great! I will do that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162442176
RFR: 8360045: StringTokenizer.hasMoreTokens() throws NPE after nextToken(null)
Fixing the side-effect caused by calling `StringTokenizer.nextToken(null)`, where the delimiter is set to `null` even if the method throws an NPE. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/25942/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25942&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8360045 Stats: 48 lines in 2 files changed: 47 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25942.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25942/head:pull/25942 PR: https://git.openjdk.org/jdk/pull/25942
Re: RFR: 8358533: Improve performance of java.io.Reader.readAllLines
On Thu, 19 Jun 2025 10:57:52 GMT, Markus KARG wrote:
>>> I think we should treat "\r\n" as a single line terminator?
>>
>> You are correct: that needs to be fixed:
>>
>> jshell> Reader r = new StringReader("hello\r\nworld")
>> r ==> java.io.StringReader@480bdb19
>>
>> jshell> r.readAllLines()
>> $3 ==> [hello, , world]
>>
>> Thanks for the catch!
>
> `Scanner` seems to scan for even more characters:
> https://github.com/openjdk/jdk/blob/c4fb00a7be51c7a05a29d3d57d787feb5c698ddf/src/java.base/share/classes/java/util/Scanner.java#L490
>
> Would it make sense to resemble this? Would it make sense to simply use
> `Scanner` directly? 🤔
The `readAllLines` method has a specification of line terminators that agrees
with that of `BufferedReader::readLine` and `String::lines` and so we don't
want to change it to be different.
Unfortunately `Scanner` doesn't seem to have a specification of what it
considers to be a line. Also unfortunately, its notion of line separators isn't
the same as the regex pattern `\R`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r2162547839
Re: RFR: 8359437: Make users and test suite not able to set LockingMode flag
On Tue, 17 Jun 2025 08:39:49 GMT, Anton Artemov wrote: > This PR contains changes for the 1st phase of the `LockingMode` flag > obsoletion. > > The work is done by @fbredber, I have taken it over and am finishing it while > he's on vacation. > > In the 1st phase one keeps the `LockingMode` variable in all places, but > makes it non-settable from the command line. All the C1 and C2 code related > to legacy locking will still be in place (but as dead code) and removed later > (phase 2). > > Lightweight locking is the default locking from now on. > > Tested in tiers 1 - 7. Marked as reviewed by lmesnik (Reviewer). Sorry, I mistakenly approved the fix instead fo request changes. Please find my change requests in previous comments. test/hotspot/jtreg/gtest/LockStackGtests.java line 26: > 24: > 25: /* @test > 26: * @summary Run LockStack gtests with LockingMode=2 All gtests are executed with default vm flags in GTestWrapper.java so this while test should be just removed. test/hotspot/jtreg/runtime/Monitor/ConcurrentDeflation.java line 37: > 35: * @bug 8318757 > 36: * @summary Test concurrent monitor deflation by MonitorDeflationThread > and thread dumping > 37: * @library /test/lib / The '/' shouldn't be required for whitebox. Could you please remove it. - PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2950786910 Changes requested by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25847#pullrequestreview-2950827379 PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2162077784 PR Review Comment: https://git.openjdk.org/jdk/pull/25847#discussion_r2162055605
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Mon, 23 Jun 2025 17:38:58 GMT, Johannes Döbler wrote: >> What would be the correct translation of majorticks here? > > If "major tick" is translated to "Hauptteilstrich", then the plural would be > `[SliderDemo.majorticks=]Hauptteilstriche` (which is a rather artificial word > so the translation team might want to come up with a better translation for > "major tick"). Microsoft uses *Hauptteilstrich*, see [MajorTickMark Klasse](https://learn.microsoft.com/de-de/dotnet/api/documentformat.openxml.drawing.charts.majortickmark?view=openxml-3.0.1), so *Hauptteilstrich* (singular Nominative) seems correct. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2162350461
