Re: RFR: 8274525: Replace uses of StringBuffer with StringBuilder in java.xml
On Wed, 29 Sep 2021 17:56:49 GMT, Andrey Turbanov wrote: > StringBuffer is a legacy synchronized class. There are more modern > alternatives which perform better: > 1. Plain String concatenation should be preferred > 2. StringBuilder is a direct replacement to StringBuffer which generally have > better performance > > Similar cleanups: > 1. [JDK-8264029](https://bugs.openjdk.java.net/browse/JDK-8264029) java.base > 2. [JDK-8264428](https://bugs.openjdk.java.net/browse/JDK-8264428) > java.desktop > 3. [JDK-8264484](https://bugs.openjdk.java.net/browse/JDK-8264484) > jdk.hotspot.agent Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5759
Re: RFR: 8274544: Langtools command's usage were grabled on Japanese Windows
On Thu, 30 Sep 2021 10:10:31 GMT, Ichiroh Takiguchi
wrote:
> * Using native.encoding system property. But
> test/langtools/tools/javac/diags/CheckResourceKeys.java was failed.
What was the cause of the failure?
> * Use java.io.Console, like Console cons = System.console() and
> cons.charset(), but "javac 2>&1 | more" does not work as expected because
> System.console() returns null.
>
> So I added -Dfile.encoding=COMPAT expect java and javaw commands on launcher.
I think we should fix the root cause of this, instead of specifying
`file.encoding=COMPAT`
>
> jshell does not work as expected on b12
Do you mean `b13`?
>
> ```
> >jdk-18-b12\bin\jshell.exe
> | JShellへようこそ -- バージョン18-ea
> | 概要については、次を入力してください: /help intro
>
> jshell> "\u3042".getBytes()
> $1 ==> byte[2] { -126, -96 }
> ```
>
> on b13
>
> ```
> >jdk-18-b13\bin\jshell.exe
> | JShellへようこそ -- バージョン18-ea
> | 概要については、次を入力してください: /help intro
>
> jshell> "\u3042".getBytes()
> $1 ==> byte[3] { -29, -127, -126 }
> ```
>
> It's UTF-8, not native encoding. I think backend java process should use same
> fine.encoding system property setting.
No it should not. `file.encoding` should not be inherited.
Naoto
-
PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Sat, 25 Sep 2021 03:38:24 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which proposes to fix the issue >> reported in https://bugs.openjdk.java.net/browse/JDK-8273790? >> >> As noted in that issue, trying to class load >> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` >> concurrently in separate threads can lead to a deadlock because of the >> cyclic nature of the code in their static initialization. More specifically, >> consider this case: >> >> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. >> - This gives T1 the implicit class init lock on `CalendarSystem`. >> - Consider thread T2 which at the same time initiates a classload on >> `sun.util.calendar.Gregorian` class. >> - This gives T2 a implicit class init lock on `Gregorian`. >> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` >> since it wants to create a (singleton) instance of `Gregorian` and assign it >> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class >> init lock on `Gregorian`, T1 ends up waiting >> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` >> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, >> T2 starts travelling up the class hierarchy and asks for a lock on >> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up >> waiting on T1 which is waiting on T2. That triggers this deadlock. >> >> The linked JBS issue has a thread dump which shows this in action. >> >> The commit here delays the instance creation of `Gregorian` by moving that >> instance creation logic from the static initialization of the >> `CalendarSystem` class, to the first call to >> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` >> from needing a lock on `Gregorian` during its static init (of course, unless >> some code in this static init flow calls >> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square >> one. I have verified, both manually and through the jtreg test, that the >> code in question doesn't have such calls) >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. The test in addition to loading these 2 classes in question, also >> additionally loads a few other classes concurrently. These classes have >> specific static initialization which leads the calls to >> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. >> Including these classes in the tests ensures that this deadlock hasn't >> "moved" to a different location. I have run multiple runs (approximately 25) >> of this test with the fix and I haven't seen it deadlock anymore. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor test adjustments as suggested by Naoto > - use a holder instead of volatile, as suggested by Roger Looks good. Thank you for the fix! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. core library part of the changes looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem
On Fri, 24 Sep 2021 14:36:07 GMT, Jaikiran Pai wrote:
> Can I please get a review for this change which proposes to fix the issue
> reported in https://bugs.openjdk.java.net/browse/JDK-8273790?
>
> As noted in that issue, trying to class load
> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian`
> concurrently in separate threads can lead to a deadlock because of the cyclic
> nature of the code in their static initialization. More specifically,
> consider this case:
>
> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`.
> - This gives T1 the implicit class init lock on `CalendarSystem`.
> - Consider thread T2 which at the same time initiates a classload on
> `sun.util.calendar.Gregorian` class.
> - This gives T2 a implicit class init lock on `Gregorian`.
> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian`
> since it wants to create a (singleton) instance of `Gregorian` and assign it
> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class
> init lock on `Gregorian`, T1 ends up waiting
> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian`
> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class,
> T2 starts travelling up the class hierarchy and asks for a lock on
> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up
> waiting on T1 which is waiting on T2. That triggers this deadlock.
>
> The linked JBS issue has a thread dump which shows this in action.
>
> The commit here delays the instance creation of `Gregorian` by moving that
> instance creation logic from the static initialization of the
> `CalendarSystem` class, to the first call to
> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem`
> from needing a lock on `Gregorian` during its static init (of course, unless
> some code in this static init flow calls
> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square
> one. I have verified, both manually and through the jtreg test, that the code
> in question doesn't have such calls)
>
> A new jtreg test has been introduced to reproduce the issue and verify the
> fix. The test in addition to loading these 2 classes in question, also
> additionally loads a few other classes concurrently. These classes have
> specific static initialization which leads the calls to
> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`.
> Including these classes in the tests ensures that this deadlock hasn't
> "moved" to a different location. I have run multiple runs (approximately 25)
> of this test with the fix and I haven't seen it deadlock anymore.
Thanks, Jaikiran. Looks good. Some minor comments.
src/java.base/share/classes/sun/util/calendar/CalendarSystem.java line 123:
> 121: */
> 122: public static Gregorian getGregorianCalendar() {
> 123: var gCal = GREGORIAN_INSTANCE;
Do we need the local variable `gCal`?
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 43:
> 41: * @run main/othervm CalendarSystemDeadLockTest
> 42: * @run main/othervm CalendarSystemDeadLockTest
> 43: * @run main/othervm CalendarSystemDeadLockTest
Just curious, before the fix, how many test instances caused the deadlock? I'd
think these 5 runs are arbitrary numbers, Just wanted to have those 5 runs are
appropriate.
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 75:
> 73: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
> 74: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
> 75: final ExecutorService executor =
> Executors.newFixedThreadPool(tasks.size());
Asserting `tasks.size() == numTasks` may help here.
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 171:
> 169: }
> 170: }
> 171: }
Need a new line at the EOF.
-
PR: https://git.openjdk.java.net/jdk/pull/5683
Integrated: 8273546: DecimalFormat documentation contains literal HTML character references
On Tue, 21 Sep 2021 21:45:40 GMT, Naoto Sato wrote: > Simple doc fix. This pull request has now been integrated. Changeset: c4345285 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/c43452859d7211f0d6537d71bd0df89412d4ff6f Stats: 14 lines in 3 files changed: 0 ins; 0 del; 14 mod 8273546: DecimalFormat documentation contains literal HTML character references Reviewed-by: joehw, bpb, iris, lancea - PR: https://git.openjdk.java.net/jdk/pull/5620
Re: RFR: 8269850: Most JDK releases report macOS version 12 as 10.16 instead of 12.0 [v2]
On Wed, 22 Sep 2021 17:41:18 GMT, Roger Riggs wrote: >> The Mac OS specific code to determine the os.version property in >> java_props_macosx.c is updated >> to replace the code extracting the version from the SystemVersion.plist by >> reading the version using t\ >> he hidden link: > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Check for missing version from SystemVersion plist. > Refactor avoid repetition of conversion of NSString to char *. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5633
Re: RFR: 8269850: Most JDK releases report macOS version 12 as 10.16 instead of 12.0
On Wed, 22 Sep 2021 15:00:59 GMT, Roger Riggs wrote:
> The Mac OS specific code to determine the os.version property in
> java_props_macosx.c is updated
> to replace the code extracting the version from the SystemVersion.plist by
> reading the version using t\
> he hidden link:
src/java.base/macosx/native/libjava/java_props_macosx.c line 270:
> 268:
> @"/System/Library/CoreServices/.SystemVersionPlatform.plist"];
> 269: if (version != NULL) {
> 270: NSString *nsVerStr = [version objectForKey :
> @"ProductVersion"];
Should we check `nsVerStr != NULL` here?
-
PR: https://git.openjdk.java.net/jdk/pull/5633
Re: Unused HashMap's in ThaiBuddhistChronology
I would think these are some left-overs from the 310 project, as I see
the same fields in 310's Hijrah chronology. These should be cleaned up.
https://github.com/ThreeTen/threetenbp/blob/master/src/main/java/org/threeten/bp/chrono/HijrahChronology.java
BTW, it would be moot once they are gone, but I found a typo:
ERA_FULL_NAMES.put(FALLBACK_LANGUAGE, new String[]{"Before
Buddhist", "Budhhist Era"});
"Budhhist" -> "Buddhist"
Naoto
On 9/21/21 2:30 PM, Roger Riggs wrote:
Indeed, they seem unused...
Stephen, do you recall their original use?
Thanks, Roger
On 9/21/21 5:09 PM, Andrey Turbanov wrote:
Hello.
Today I discovered 3 static HashMap's in ThaiBuddhistChronology class:
ERA_NARROW_NAMES, ERA_SHORT_NAMES, ERA_FULL_NAMES
static initializer put some values into them. But their content seems
unused after that.
Do I miss something and they are used somewhere?
Andrey Turbanov
Integrated: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add()
On Tue, 21 Sep 2021 12:47:00 GMT, Naoto Sato wrote: > Fixing an AIOOBE on normalizing the month value. This pull request has now been integrated. Changeset: d39aad92 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/d39aad92308fbc28bd2de164e331062ebf62da85 Stats: 26 lines in 3 files changed: 24 ins; 0 del; 2 mod 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add() Reviewed-by: rriggs, iris, joehw - PR: https://git.openjdk.java.net/jdk/pull/5611
Re: RFR: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add() [v2]
> Fixing an AIOOBE on normalizing the month value. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed the unnecessary space - Changes: - all: https://git.openjdk.java.net/jdk/pull/5611/files - new: https://git.openjdk.java.net/jdk/pull/5611/files/92f81963..98612bf5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5611=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5611=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5611.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5611/head:pull/5611 PR: https://git.openjdk.java.net/jdk/pull/5611
Re: RFR: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add() [v2]
On Tue, 21 Sep 2021 22:03:30 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed the unnecessary space
>
> src/java.base/share/classes/sun/util/calendar/BaseCalendar.java line 281:
>
>> 279: month = 13 - (xm % 12);
>> 280: if (month == 13) {
>> 281: year ++;
>
> Not that it matters, just happen to see it ;-). Is it more common to have no
> space before the increment operator? Code convention seems to suggest no
> space before increment and their operands.
> No update needed should you decide to remove it.
Thanks. Indeed it was in the coding convention. Removed.
-
PR: https://git.openjdk.java.net/jdk/pull/5611
RFR: 8273546: DecimalFormat documentation contains literal HTML character references
Simple doc fix. - Commit messages: - 8273546: DecimalFormat documentation contains literal HTML character references Changes: https://git.openjdk.java.net/jdk/pull/5620/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5620=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273546 Stats: 14 lines in 3 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/5620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5620/head:pull/5620 PR: https://git.openjdk.java.net/jdk/pull/5620
Re: RFR: 8274003: ProcessHandleImpl.Info toString has an if check which is always true
On Tue, 21 Sep 2021 18:14:17 GMT, Roger Riggs wrote: > Correct the check if any field has been appended to the StringBuilder in > ProcessHandleImpl.Info.toString(). Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5618
RFR: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add()
Fixing an AIOOBE on normalizing the month value. - Commit messages: - 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add() Changes: https://git.openjdk.java.net/jdk/pull/5611/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5611=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273924 Stats: 26 lines in 3 files changed: 24 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5611.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5611/head:pull/5611 PR: https://git.openjdk.java.net/jdk/pull/5611
Integrated: 8273187: jtools tests fail with missing markerName check
On Thu, 16 Sep 2021 01:08:45 GMT, Naoto Sato wrote: > Fixing failing regression tests caused by the JEP 400: UTF-8 by Default. > > `JcmdOutputEncodingTest` test case uses `file.encoding=UTF-8` in `C` locale. > The output from the agent library is in `UTF-8` so it succeeded before the > JEP has been implemented, as System.out used `UTF-8` converter. After the > JEP, it started failing because System.out is using `US-ASCII` which > generates series of '?', ending up with assertion failure in the test. > > The proposed fix is to pass `sun.stdout.encoding=UTF-8` as well as > `file.encoding` so that tool process' System.out is in `UTF-8` as well. This pull request has now been integrated. Changeset: f71df142 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/f71df142a97f522b40e90c3105e0e5bd8d5af9a2 Stats: 10 lines in 3 files changed: 3 ins; 4 del; 3 mod 8273187: jtools tests fail with missing markerName check Reviewed-by: iris, sspitsyn, joehw - PR: https://git.openjdk.java.net/jdk/pull/5539
RFR: 8273187: jtools tests fail with missing markerName check
Fixing failing regression tests caused by the JEP 400: UTF-8 by Default. `JcmdOutputEncodingTest` test case uses `file.encoding=UTF-8` in `C` locale. The output from the agent library is in `UTF-8` so it succeeded before the JEP has been implemented, as System.out used `UTF-8` converter. After the JEP, it started failing because System.out is using `US-ASCII` which generates series of '?', ending up with assertion failure in the test. The proposed fix is to pass `sun.stdout.encoding=UTF-8` as well as `file.encoding` so that tool process' System.out is in `UTF-8` as well. - Commit messages: - Merge branch 'master' into JDK-8273187 - Reverted command encoding, then modified the test case to use sun.stdout.encoding - Used `transferTo()` - Merge branch 'master' into JDK-8273187 - Nuked UTF-8 conversion in tools - Merge branch 'master' into JDK-8273187 - 8273187: jtools tests fail with missing markerName check Changes: https://git.openjdk.java.net/jdk/pull/5539/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5539=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273187 Stats: 10 lines in 3 files changed: 3 ins; 4 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5539.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5539/head:pull/5539 PR: https://git.openjdk.java.net/jdk/pull/5539
Integrated: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
On Thu, 9 Sep 2021 23:29:24 GMT, Naoto Sato wrote: > Small spec clarification. Corresponding CSR has also been drafted. This pull request has now been integrated. Changeset: 31667daa Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/31667daa50b2faf82943821ee02071d222e38268 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict Reviewed-by: joehw, lancea - PR: https://git.openjdk.java.net/jdk/pull/5457
Integrated: 8273259: Character.getName doesn't follow Unicode spec for ideographs
On Thu, 2 Sep 2021 19:26:12 GMT, Naoto Sato wrote: > Simple spec clarification. A CSR has also been drafted > (https://bugs.openjdk.java.net/browse/JDK-8273296). This pull request has now been integrated. Changeset: 4cfa230e Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/4cfa230e2daac118f21c7d8996d48a1a15d87a62 Stats: 21 lines in 1 file changed: 12 ins; 0 del; 9 mod 8273259: Character.getName doesn't follow Unicode spec for ideographs Reviewed-by: bpb, lancea, iris, darcy - PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict [v2]
On Fri, 10 Sep 2021 20:53:35 GMT, Joe Wang wrote: >> Hi Naoto, >> >> A couple of questions: >> >> - Are there any scenarios where invoking setProperty will not override the >> command line setting ? >> - Did you consider an `@ImplNote` for your clarification given the behavior >> "might" be different in other implementations (I am not sure myself) and is >> implementation defined? >> >> Best >> Lance > > That sounds good to me, suggesting doing it at launch while discouraging the > use of setProperty. Hi Lance, > * Are there any scenarios where invoking setProperty will not override the > command line setting ? Yes. For example, if a logger is installed, it formats the log date/time which causes initialization of locale-related classes before the app's main() method invocation, resulting setProperty() having no effect. > * Did you consider an `@ImplNote` for your clarification given the behavior > "might" be different in other implementations (I am not sure myself) and is > implementation defined? As in the above example, the behavior changes regardless of the implementation of the runtime. The clarification is mainly for the API consumer, rather than platform implementors. Hope this answers your questions. - PR: https://git.openjdk.java.net/jdk/pull/5457
Re: RFR: 8273430: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark [v2]
On Fri, 10 Sep 2021 20:57:34 GMT, Ian Graves wrote: >> The duplicate condition in this chain of expressions needs to be shrunk to >> drop a couple of character that are not excluded spacing marks. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Refactoring test to whitebox Looks good. Nice cleanup! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5425
Re: RFR: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict [v2]
On Fri, 10 Sep 2021 18:11:37 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review comment.
>
> src/java.base/share/classes/java/util/spi/LocaleServiceProvider.java line 120:
>
>> 118: * the locale sensitive services separated by a comma. It is only read
>> and cached at
>> 119: * the initialization of this class, so the later call to
>> 120: * {@link System#setProperty(String, String)} may not affect the order.
>
> I wonder if we can be clearer as "may not" implies uncertainty. While it
> indeed may or may not work due to the timing of the initialization of this
> class, my understanding of the above statement is that it implied the runtime
> startup is recommended as it provides assurance. Would it be better to put
> that in the statement? sth. like: It is read once and cached at the Java
> runtime startup or initialization of this class. A call after the
> initialization of this class will not affect the order.
It was intentional to use `may not` because as you said, there's still
uncertainty. To clarify it more, I added wording that `setProperty` use is
discouraged to change the preferred order.
-
PR: https://git.openjdk.java.net/jdk/pull/5457
Re: RFR: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict [v2]
> Small spec clarification. Corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflecting review comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5457/files - new: https://git.openjdk.java.net/jdk/pull/5457/files/2781ad32..70bfbd69 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5457=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5457=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5457.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5457/head:pull/5457 PR: https://git.openjdk.java.net/jdk/pull/5457
Integrated: 8273369: Computing micros between two instants unexpectedly overflows for some cases
On Tue, 7 Sep 2021 18:18:49 GMT, Naoto Sato wrote: > Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. This pull request has now been integrated. Changeset: 81d2acee Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/81d2acee57188a4507c798b46b0bd129dc302fec Stats: 52 lines in 3 files changed: 46 ins; 0 del; 6 mod 8273369: Computing micros between two instants unexpectedly overflows for some cases Reviewed-by: lancea, rriggs, joehw - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273259: Character.getName doesn't follow Unicode spec for ideographs [v3]
> Simple spec clarification. A CSR has also been drafted > (https://bugs.openjdk.java.net/browse/JDK-8273296). Naoto Sato 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 four additional commits since the last revision: - Merge branch 'master' into JDK-8273259 - Reflecting the CSR review comment - Refined wordings. - 8273259: Character.getName doesn't follow Unicode spec for ideographs - Changes: - all: https://git.openjdk.java.net/jdk/pull/5354/files - new: https://git.openjdk.java.net/jdk/pull/5354/files/dc36e741..04034295 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5354=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5354=01-02 Stats: 12617 lines in 569 files changed: 7866 ins; 2572 del; 2179 mod Patch: https://git.openjdk.java.net/jdk/pull/5354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5354/head:pull/5354 PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8273513: Make java.io.FilterInputStream specification more precise about overrides [v2]
On Thu, 9 Sep 2021 23:54:25 GMT, Brian Burkhalter wrote:
>> Modify the class level specification of `java.io.FilterInputStream` to be
>> more exact about `java.io.InputStream` methods that it overrides.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8273513: Change key to select as suggested in CSR comment
LGTM.
src/java.base/share/classes/java/io/FilterInputStream.java line 90:
> 88: * This method simply performs the call
> 89: * {@code read(b, 0, b.length)} and returns
> 90: * the result. It is important that it does
Preexisting: an extra space before "result."
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5426
RFR: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
Small spec clarification. Corresponding CSR has also been drafted. - Commit messages: - 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict Changes: https://git.openjdk.java.net/jdk/pull/5457/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5457=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273491 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5457.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5457/head:pull/5457 PR: https://git.openjdk.java.net/jdk/pull/5457
Integrated: 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed"
On Wed, 8 Sep 2021 22:15:12 GMT, Naoto Sato wrote: > The gist of the issue is that the test case now always creates the boot > classpath with non-ASCII chars appended, because the default encoding is > fixed to UTF-8 with the fix to JDK-8260265. > > On macOS, javaagent tries to load the class with US-ASCII determined by > nl_langinfo() (in unix/.../EncodingSupport_md.c). The file system encoding is > always UTF-8 on mac, so no need to use the determined encoding by > nl_langinfo(). > > On Linux, the "defaultEncoding" in Setup.java used to be US-ASCII in "C" > locale (which matches the result from nl_langinfo()), so no non-ASCII suffix > was appended to the `boot` path. But now the "defaultEncoding" is always > UTF-8, the setup code appends the non-ASCII suffix, which fails to read in > the native code using iconv with US-ASCII. The setup code should use the > encoding from "sun.jnu.encoding" instead. Actually, the code there was copied > from UnicodeTest.java which was modified with JDK-8260265, so the same fix > needs to be applied. > > Tier5 run succeeds with one unrelated failure. Also, fixed some typos, e.g. > "Uft8" -> "Utf8" This pull request has now been integrated. Changeset: 54dee132 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/54dee132d1a149165e7478b29b740d086c18c424 Stats: 36 lines in 8 files changed: 5 ins; 16 del; 15 mod 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed" Reviewed-by: dholmes, alanb - PR: https://git.openjdk.java.net/jdk/pull/5427
Re: RFR: 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed"
On Wed, 8 Sep 2021 23:38:38 GMT, David Holmes wrote: > Pre-existing: The initialization logic in this code is quite odd for the case > when no conversion is necessary (we call `utfInitialize` on every call to > `convertUtf8ToPlatformString`!), but I assume we do not call > `appendBootClassPath` and `appendToClassLoaderSearch` very often? Indeed. I assume those methods are only used on agent startup. - PR: https://git.openjdk.java.net/jdk/pull/5427
Re: RFR: 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed"
On Wed, 8 Sep 2021 22:15:12 GMT, Naoto Sato wrote: > The gist of the issue is that the test case now always creates the boot > classpath with non-ASCII chars appended, because the default encoding is > fixed to UTF-8 with the fix to JDK-8260265. > > On macOS, javaagent tries to load the class with US-ASCII determined by > nl_langinfo() (in unix/.../EncodingSupport_md.c). The file system encoding is > always UTF-8 on mac, so no need to use the determined encoding by > nl_langinfo(). > > On Linux, the "defaultEncoding" in Setup.java used to be US-ASCII in "C" > locale (which matches the result from nl_langinfo()), so no non-ASCII suffix > was appended to the `boot` path. But now the "defaultEncoding" is always > UTF-8, the setup code appends the non-ASCII suffix, which fails to read in > the native code using iconv with US-ASCII. The setup code should use the > encoding from "sun.jnu.encoding" instead. Actually, the code there was copied > from UnicodeTest.java which was modified with JDK-8260265, so the same fix > needs to be applied. > > Tier5 run succeeds with one unrelated failure. Also, fixed some typos, e.g. > "Uft8" -> "Utf8" Hi David, > Doesn't it depend on the OS version and which file system is being used? HFS+ > seems to use UTF-16 Right. I was not clear enough as to `file system encoding`, what I meant was the `sun.jnu.encoding`, which is always `UTF-8` with this change: https://bugs.openjdk.java.net/browse/JDK-8003228 - PR: https://git.openjdk.java.net/jdk/pull/5427
RFR: 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed"
The gist of the issue is that the test case now always creates the boot classpath with non-ASCII chars appended, because the default encoding is fixed to UTF-8 with the fix to JDK-8260265. On macOS, javaagent tries to load the class with US-ASCII determined by nl_langinfo() (in unix/.../EncodingSupport_md.c). The file system encoding is always UTF-8 on mac, so no need to use the determined encoding by nl_langinfo(). On Linux, the "defaultEncoding" in Setup.java used to be US-ASCII in "C" locale (which matches the result from nl_langinfo()), so no non-ASCII suffix was appended to the `boot` path. But now the "defaultEncoding" is always UTF-8, the setup code appends the non-ASCII suffix, which fails to read in the native code using iconv with US-ASCII. The setup code should use the encoding from "sun.jnu.encoding" instead. Actually, the code there was copied from UnicodeTest.java which was modified with JDK-8260265, so the same fix needs to be applied. Tier5 run succeeds with one unrelated failure. Also, fixed some typos, e.g. "Uft8" -> "Utf8" - Commit messages: - Merge branch 'master' into JDK-8273188 - 8273188: java/lang/instrument/BootClassPath/BootClassPathTest.sh fails with "FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed" Changes: https://git.openjdk.java.net/jdk/pull/5427/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5427=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273188 Stats: 36 lines in 8 files changed: 5 ins; 16 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/5427.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5427/head:pull/5427 PR: https://git.openjdk.java.net/jdk/pull/5427
Re: RFR: 8273430: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
On Wed, 8 Sep 2021 20:24:31 GMT, Ian Graves wrote: > The duplicate condition in this chain of expressions needs to be shrunk to > drop a couple of character that are not excluded spacing marks. The copyright year in Grapheme.java should be 2021, otherwise looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5425
Re: RFR: 8273242: (test) Refactor to use TestNG for RuntimeTests ExecCommand tests [v2]
On Wed, 8 Sep 2021 19:31:46 GMT, Roger Riggs wrote: >> The ExecCommand test of Runtime.exec is difficult to maintain; the parallel >> arrays are hard to keep in sync. >> This cleanup converts to use TestNG DataProviders and other improvements. > > Roger Riggs 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 three additional > commits since the last revision: > > - Simplify file deletion >Add enum to document test modes. > - Merge branch 'master' into 8273242-execcommand-refactor > - 8273242: Refactored ExecCommand to use TestNG DataProvider for cases Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5335
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]
On Wed, 8 Sep 2021 15:35:27 GMT, Stephen Colebourne wrote: > `toEpochMilli()` overflows at large/small values. Thus any attempt to > calculate the difference between two very large instants would fail. Thanks. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v6]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Corrected the calculation for MILLIS as well. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/ccf73bf7..1f6fd470 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5396=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5396=04-05 Stats: 24 lines in 3 files changed: 21 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]
On Wed, 8 Sep 2021 13:58:59 GMT, Stephen Colebourne wrote: > This change looks fine, but I think you also need a `millisUntil` private > method to fix the identical overflow problem with millis (which might as well > be fixed now). `until()` for millis simply subtracts its `toEpochMilli()` from the end Instant's, so I am not sure that would cause the same false `ArithmeticException`. Can you please elaborate more? - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273259: Character.getName doesn't follow Unicode spec for ideographs [v2]
> Simple spec clarification. A CSR has also been drafted > (https://bugs.openjdk.java.net/browse/JDK-8273296). Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined wordings. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5354/files - new: https://git.openjdk.java.net/jdk/pull/5354/files/294e3d72..dc36e741 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5354=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5354=00-01 Stats: 10 lines in 1 file changed: 4 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5354/head:pull/5354 PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
On Wed, 8 Sep 2021 00:15:46 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the constant to LocalTime for consistency.
>
> test/jdk/java/time/test/java/time/TestInstant.java line 102:
>
>> 100:
>> 101: @Test
>> 102: public void test_microsUntil() {
>
> A comment about the test might be helpful.
Indeed. Added some comments to the test.
-
PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comments for the test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/bbd6abc3..ccf73bf7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5396=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5396=03-04 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273242: (test) Refactor to use TestNG for RuntimeTests ExecCommand tests
On Wed, 1 Sep 2021 16:37:24 GMT, Roger Riggs wrote: > The ExecCommand test of Runtime.exec is difficult to maintain; the parallel > arrays are hard to keep in sync. > This cleanup converts to use TestNG DataProviders and other improvements. LGTM. test/jdk/java/lang/RuntimeTests/exec/ExecCommand.java line 270: > 268: } > 269: } > 270: } No newline at the eof. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5335
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v4]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Moved the constant to LocalTime for consistency. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/7d567e8b..bbd6abc3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5396=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5396=02-03 Stats: 10 lines in 2 files changed: 5 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v3]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Corrected the constant. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/4b874ff6..7d567e8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5396=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5396=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v2]
On Tue, 7 Sep 2021 18:46:56 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added a constant for nanos per micro. > > src/java.base/share/classes/java/time/Instant.java line 1170: > >> 1168: long secsDiff = Math.subtractExact(end.seconds, seconds); >> 1169: long totalMicros = Math.multiplyExact(secsDiff, >> NANOS_PER_SECOND / 1000); >> 1170: return Math.addExact(totalMicros, (end.nanos - nanos) / 1000); > > Can you define NANOS_PER_MICRO, the others conversions use predefined > constants. Defined it as a private field in `Instant`. - PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v2]
> Please review the fix to the issue. Avoiding overflow by not calling > nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference > in nano unit. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a constant for nanos per micro. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5396/files - new: https://git.openjdk.java.net/jdk/pull/5396/files/09d9b257..4b874ff6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5396=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5396=00-01 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases
Please review the fix to the issue. Avoiding overflow by not calling nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference in nano unit. - Commit messages: - 8273369: Computing micros between two instants unexpectedly overflows for some cases Changes: https://git.openjdk.java.net/jdk/pull/5396/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5396=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273369 Stats: 17 lines in 2 files changed: 15 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5396/head:pull/5396 PR: https://git.openjdk.java.net/jdk/pull/5396
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v5]
On Tue, 7 Sep 2021 11:26:01 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: some tests in jdk/tools/launcher/ fails on localized Windows > platform Thanks. Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4594
Re: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
It does look incorrect. I will take a look.
Naoto
On 9/6/21 12:16 AM, Andrey Turbanov wrote:
Hello.
I found suspicious condition in the method
java.util.regex.Grapheme#isExcludedSpacingMark
It's detected by IntelliJ IDEA inspection 'Condition is covered by
further condition'
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Grapheme.java#L157
```
private static boolean isExcludedSpacingMark(int cp) {
return cp == 0x102B || cp == 0x102C || cp == 0x1038 ||
cp >= 0x1062 && cp <= 0x1064 ||
cp >= 0x1062 && cp <= 0x106D || // <== here is the warning
cp == 0x1083 ||
cp >= 0x1087 && cp <= 0x108C ||
cp == 0x108F ||
cp >= 0x109A && cp <= 0x109C ||
cp == 0x1A61 || cp == 0x1A63 || cp == 0x1A64 ||
cp == 0xAA7B || cp == 0xAA7D;
}
```
There are 2 sub-conditions in this complex condition:
cp >= 0x1062 && cp <= 0x1064 ||
cp >= 0x1062 && cp <= 0x106D ||
The second condition is _wider_ than the first one.
I believe it's a bug. The second condition (according to
https://www.compart.com/en/unicode/category/Mc) should look like this:
cp >= 0x1067 && cp <= 0x106D ||
0x1065, 0x1066 are not from the Spacing_Mark category.
Andrey Turbanov
Re: RFR: 8273329: Remove redundant null check from String.getBytes(String charsetName)
On Fri, 3 Sep 2021 13:22:54 GMT, Сергей Цыпанов
wrote:
> Current implementation looks like this:
>
> public byte[] getBytes(String charsetName)
> throws UnsupportedEncodingException {
> if (charsetName == null) throw new NullPointerException();
> return encode(lookupCharset(charsetName), coder(), value);
> }
>
> Null check seems to be redundant here because the same check of `charsetName`
> is done within `String.lookupCharset(String)`:
>
> private static Charset lookupCharset(String csn) throws
> UnsupportedEncodingException {
> Objects.requireNonNull(csn);
> try {
> return Charset.forName(csn);
> } catch (UnsupportedCharsetException | IllegalCharsetNameException x) {
> throw new UnsupportedEncodingException(csn);
> }
> }
Looks good. Please add some `noreg` keyword to the JIRA issue.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5361
RFR: 8273259: Character.getName doesn't follow Unicode spec for ideographs
Simple spec clarification. A CSR has also been drafted (https://bugs.openjdk.java.net/browse/JDK-8273296). - Commit messages: - 8273259: Character.getName doesn't follow Unicode spec for ideographs Changes: https://git.openjdk.java.net/jdk/pull/5354/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5354=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273259 Stats: 19 lines in 1 file changed: 8 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/5354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5354/head:pull/5354 PR: https://git.openjdk.java.net/jdk/pull/5354
Re: RFR: 8273250: Address javadoc issues in Deflater::setDictionationary
On Wed, 1 Sep 2021 19:26:17 GMT, Lance Andersen wrote: > Hi, > > Please review this trivial fix to the javadoc which addresses an issue shown > via Intellij where the error: "Symbol 'getAdler' is inaccessible from here" > is generated for the "@See Inflater#getAlder" references. > > Best > Lance Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5340
Re: RFR: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
On Wed, 1 Sep 2021 17:33:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5337
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan The change looks reasonable. Please test your fix with macOS as well. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: Integrated: 8273197: ProblemList 2 jtools tests due to JDK-8273187
On Tue, 31 Aug 2021 19:44:08 GMT, Daniel D. Daugherty wrote: > Trivial fixes to reduce the noise in the JDK18 CI: > JDK-8273197 ProblemList 2 jtools tests due to JDK-8273187 > JDK-8273198 ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 > > These failures happen in Tier5 so I'm ProblemListing them now to give @naotoj > time to > work on the issues introduced by JDK-8260265 UTF-8 by Default. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5321
Integrated: 8260265: UTF-8 by Default
On Thu, 8 Jul 2021 21:23:00 GMT, Naoto Sato wrote: > This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 This pull request has now been integrated. Changeset: 7fc85409 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/7fc8540907e8e7483ad5729ea416167810aa8747 Stats: 409 lines in 22 files changed: 208 ins; 24 del; 177 mod 8260265: UTF-8 by Default Reviewed-by: alanb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v9]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed a failing test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/67e1d4a9..28e79d4e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=07-08 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v8]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato 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 15 additional commits since the last revision: - Removed "default" alias - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Refined `file.encoding` description - Merge branch 'master' into JDK-8260265 - Reflects comments - ... and 5 more: https://git.openjdk.java.net/jdk/compare/66c3531f...67e1d4a9 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/7d5137d3..67e1d4a9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=06-07 Stats: 15450 lines in 595 files changed: 11498 ins; 2067 del; 1885 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments I think implicitly expecting locales to be set to en-US by specifying `test.vm.opts` is fragile which would introduce test instability. In fact, looking at some of the tests, e.g., `HelpFlagsTest` at line 332, the intention is to silently exit in case it is not English. I think it is what the test is intended and it is a bug if it fails with other locales. - PR: https://git.openjdk.java.net/jdk/pull/4594
Integrated: 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong
On Mon, 23 Aug 2021 16:42:03 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. When instant seconds and zone > co-exist in parsed data, instant seconds was not resolved correctly from them. This pull request has now been integrated. Changeset: fe7d7088 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/fe7d70886cc9985478c5810eff0790648a9aae41 Stats: 14 lines in 2 files changed: 8 ins; 0 del; 6 mod 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong Reviewed-by: joehw, rriggs, iris, lancea, scolebourne - PR: https://git.openjdk.java.net/jdk/pull/5225
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. java.time changes look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
RFR: 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong
Please review the fix to the subject issue. When instant seconds and zone co-exist in parsed data, instant seconds was not resolved correctly from them. - Commit messages: - 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong Changes: https://git.openjdk.java.net/jdk/pull/5225/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5225=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272473 Stats: 14 lines in 2 files changed: 8 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5225.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5225/head:pull/5225 PR: https://git.openjdk.java.net/jdk/pull/5225
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments IMHO, this kind of test that sets the locale forcefully sometimes gives us false positives. I'd rather eliminate that possibility than run tests in different locales - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8272616: Strange code in java.text.DecimalFormat#applyPattern
On Wed, 18 Aug 2021 20:59:20 GMT, Andrey Turbanov wrote: > remove redundant if Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5171
Re: JEP proposed to target JDK 18: 400: UTF-8 by Default
On 8/18/21 2:03 PM, Simon Nash wrote: I am the developer of a fairly large application that uses file I/O extensively. In most cases, the charset should be UTF-8 and I have used an explicit charset parameter on all method invocations where this applies. In some cases, the charset needs to be the platform default charset to produce output that is readable by other programs or by a user (for example, Windows-1252 on some versions of Windows). In the cases that need the platform default charset, I have omitted the charset (intentionally, not carelessly or accidentally). If the behaviour changes in these cases, it will produce unexpected results for users. In preparation for JEP 400, we have provided a new system property that retrieves the native encoding name in JDK17: https://bugs.openjdk.java.net/browse/JDK-8265989 Apps that have luxury to make code base change can use the property to retrieve the native encoding, then use it to replace no-arg I/O constructors to the explicit equivalent ones. I could try to find all the method invocations that currently use the implicit default charset, although I have no idea how to do this other than reading every line of code. The problems with this are 1) that I would almost certainly miss some invocations that need to be changed and 2) more seriously, from what I have seen in the JEP I don't think there is a way to update these method invocations that works exactly as at present on all versions of Java back to JDK 8 and provides the same behaviour on JDK 18. This is because (as far as I can tell) there is no API call that returns the "old-style" platform default charset and can be used on all JDK versions from JDK 8 to JDK 18. Adding a -D option when the application is started isn't possible in some contexts such as launching the application from a Windows executable jar file association. If I could make a single API call when the application is first started to force backward-compatible behaviour in all cases, this would solve the problem. This feels very much like a "hack" and I would much prefer a clean solution but it would be better than nothing. I am reluctant to provide such a single API call which has the same effect as `COMPAT`, as it will become less meaningful when UTF-8 as default sinks in. Naoto
Re: RFR: 8260265: UTF-8 by Default [v7]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato 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 12 additional commits since the last revision: - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Refined `file.encoding` description - Merge branch 'master' into JDK-8260265 - Reflects comments - Removed leftover `console` references in `PrintStream` - Reflects comments - Reflects review comments - ... and 2 more: https://git.openjdk.java.net/jdk/compare/387b32b3...7d5137d3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/6f9e5eb4..7d5137d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=05-06 Stats: 53933 lines in 926 files changed: 45460 ins; 4145 del; 4328 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
On Thu, 12 Aug 2021 17:43:48 GMT, Lance Andersen wrote:
> Hi all,
>
> Please review the fix for JDK-8263940 to address an issues when the default
> file system provider is packaged as JAR file on class path.
>
> The patch also addresses the `@bug` line for JDK-8271194
>
> Mach5 Tier1 - Tier3 have run without issues
>
> Best,
> Lance
Looks good to me, Lance.
test/jdk/java/nio/file/spi/SetDefaultProvider.java line 107:
> 105: .map(path -> path.getFileName().toString())
> 106: .filter(f -> f.startsWith("TestProvider"))
> 107: .collect(Collectors.toList());
Nit: Could simply issue `.toList()` here.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v4]
On Wed, 11 Aug 2021 21:40:52 GMT, Claes Redestad wrote: >> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to >> avoid potentially getting two bounds checks in the UTF-16 case. Problem is >> this optimization cause a regression since `StringUTF16.charAt(..)` checks >> `index` against the length of the `value` array and not `count`. >> >> A correct fix that still maintain the possible performance advantage >> reported by #4738 is to reinstate the `checkIndex(value, count)` and call >> `StringUTF16.getChar` instead of `charAt`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copy-paste error Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Marked as reviewed by naoto (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking
On Wed, 11 Aug 2021 14:26:32 GMT, Claes Redestad wrote: > In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. LGTM. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
+1
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Integrated: 8264792: The NumberFormat for locale sq_XK formats price incorrectly.
On Fri, 6 Aug 2021 16:39:34 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. The root cause of this problem is > that the currency for the country code `XK` is undefined because the country > code is user-defined in the ISO 3166 standard. However, it is commonly used > to represent the region `Kosovo`, which CLDR supports and subsequently is > supported by the JDK since JDK9. Adding the data `EUR` for the country code > `XK` will fix the issue. This pull request has now been integrated. Changeset: 41dc795d Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/41dc795d6c08af84aa6544cc5a5704dcf99386cf Stats: 13 lines in 3 files changed: 6 ins; 0 del; 7 mod 8264792: The NumberFormat for locale sq_XK formats price incorrectly. Reviewed-by: joehw, iris - PR: https://git.openjdk.java.net/jdk/pull/5033
RFR: 8264792: The NumberFormat for locale sq_XK formats price incorrectly.
Please review the fix to the subject issue. The root cause of this problem is that the currency for the country code `XK` is undefined because the country code is user-defined in the ISO 3166 standard. However, it is commonly used to represent the region `Kosovo`, which CLDR supports and subsequently is supported by the JDK since JDK9. Adding the data `EUR` for the country code `XK` will fix the issue. - Commit messages: - 8264792: The NumberFormat for locale sq_XK formats price incorrectly. Changes: https://git.openjdk.java.net/jdk/pull/5033/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5033=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264792 Stats: 13 lines in 3 files changed: 6 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5033.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5033/head:pull/5033 PR: https://git.openjdk.java.net/jdk/pull/5033
Re: [jdk17] RFR: JDK-8270872: Final nroff manpage update for JDK 17
On Thu, 5 Aug 2021 21:36:58 GMT, Jonathan Gibbons wrote: >> src/jdk.hotspot.agent/share/man/jhsdb.1 line 1: >> >>> 1: .\" Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights >>> reserved. >> >> This seems not correct? > > According to the comments in the makefile > (`closed/make/UpdateOpenManPages.gmk`) the copyright line is taken from the > original Markdown file, so if the year is wrong there, it will be wrong in > the generated nroff file. > > I think it would be incorrect to edit the dates locally in these files, > because they'll just be overwritten when we generate the files again. > Ideally, the dates should be fixed (if necessary) in the Markdown files, but > that seems out of scope for this P1. > > This is "just" an issue with copyright dates in source files ... and yes, > while I know copyright dates are important, this problem is arguably part of > an ongoing more general problem. > > I note that the generated files *do* correctly identify themselves with > `2021` in the visible output generated to the console by the `man` command. Thanks for the explanation, Jon. Fine by me. - PR: https://git.openjdk.java.net/jdk17/pull/303
Re: [jdk17] RFR: JDK-8270872: Final nroff manpage update for JDK 17
On Thu, 5 Aug 2021 19:20:50 GMT, Jonathan Gibbons wrote: > Please review a semi-automatic update of the nroff man pages from the > upstream files. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/303
Re: [jdk17] RFR: JDK-8270872: Final nroff manpage update for JDK 17
On Thu, 5 Aug 2021 19:20:50 GMT, Jonathan Gibbons wrote: > Please review a semi-automatic update of the nroff man pages from the > upstream files. src/jdk.hotspot.agent/share/man/jhsdb.1 line 1: > 1: .\" Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights > reserved. This seems not correct? - PR: https://git.openjdk.java.net/jdk17/pull/303
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments Then I would think the better fix would be to run the test if the default locale is `Locale.US`, otherwise the test should exit gracefully. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v5]
On Fri, 30 Jul 2021 15:43:52 GMT, Lance Andersen wrote:
>> Hi,
>>
>> As discussed in the
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
>> thread, this is the revised patch to address the use of '.' and '..' within
>> Zip FS
>>
>> Zip FS needs to use "." and ".." as links to the current and parent
>> directories and cannot reliably support entries that have "." and ".." as
>> name elements. This patch updates Zip Fs to reject ZIP files that have
>> entries in the CEN that can't be used for files in a file system.
>>
>>
>> Mach5 tiers 1 through 3 have been run without any errors encountered .
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update note added to Zip FS module-info
Looks good to me.
test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 116:
> 114: @Test(dataProvider = "checkForDotOrDotDotPaths")
> 115: public void hasDotOrDotDotTest(String path) throws IOException {
> 116: if(DEBUG) {
Nit: space after `if`
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Thu, 29 Jul 2021 18:21:07 GMT, Lance Andersen wrote: >> Hi, >> >> As discussed in the >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html >> thread, this is the revised patch to address the use of '.' and '..' within >> Zip FS >> >> Zip FS needs to use "." and ".." as links to the current and parent >> directories and cannot reliably support entries that have "." and ".." as >> name elements. This patch updates Zip Fs to reject ZIP files that have >> entries in the CEN that can't be used for files in a file system. >> >> >> Mach5 tiers 1 through 3 have been run without any errors encountered . >> >> Best, >> Lance > > Lance Andersen 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 six additional > commits since the last revision: > > - Add Impl Note to Zip FS module-info > - Merge > - Add missing Copyright header and address minor comments > - Address missing linefeed after package name > - Address overzelous intellij import update > - Patch to address JDK-8251329 src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1622: > 1620: index++; > 1621: } > 1622: return dotOrDotDotFound; Could simply return `false` here, and eliminate the local variable `dotOrDotDotFound`. - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: [jdk17] RFR: 8271489: (doc) Clarify Filter Factory example
On Thu, 29 Jul 2021 19:05:58 GMT, Roger Riggs wrote: > Improve the clarity of comments in the ObjectInputFilter FilterInThread > example. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/293
Re: [jdk17] RFR: 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java
On Wed, 28 Jul 2021 17:51:31 GMT, Daniel D. Daugherty wrote: > 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/291
Integrated: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. This pull request has now been integrated. Changeset: efa63dc1 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/efa63dc1c64db357eeb497d2e1fefd170ca22d98 Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod 8171382: java.time.Duration missing isPositive method Reviewed-by: rriggs, joehw, iris, bpb, scolebourne - PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 19:37:44 GMT, Stephen Colebourne
wrote:
>> Please review this PR to introduce `java.time.Duration.isPositive()` method.
>> A CSR is also drafted.
>
> src/java.base/share/classes/java/time/Duration.java line 596:
>
>> 594: */
>> 595: public boolean isPositive() {
>> 596: return (seconds | nanos) > 0;
>
> I had to think whether this logic is correct, but I believe it is because
> `nanos` is 32 bits and positive so won't impact the negative bit of `seconds`.
Thanks, Stephen. Yes, `nanos` is even smaller, less than `1,000,000,000`.
-
PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. Thanks, Roger. Modified the CSR as suggested. - PR: https://git.openjdk.java.net/jdk/pull/4892
RFR: 8171382: java.time.Duration missing isPositive method
Please review this PR to introduce `java.time.Duration.isPositive()` method. A CSR is also drafted. - Commit messages: - 8171382: java.time.Duration missing isPositive method Changes: https://git.openjdk.java.net/jdk/pull/4892/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4892=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8171382 Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4892.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4892/head:pull/4892 PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8265474: Dubious 'null' assignment in CompactByteArray.expand
On Wed, 23 Dec 2020 16:06:30 GMT, Andrey Turbanov wrote: > I propose to remove 'null' assignment of field CompactByteArray.values in > `expand` method. In the next statement this field is overridden with another > value - `tempArray`. > This code was there from initial load of OpenJDK sources. I believe it was > just leftovers from development phase of this class. There is no practical > reason to assign `null` to non-volatile field and then overwrite it with > another value. > Also I've removed unused method `getArray`. I hope it's ok to cleanup such > unused stuff in the same PR. Yes, it does. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1880
Re: [jdk17] RFR: 8270916: Update java.lang.annotation.Target for changes in JLS 9.6.4.1
On Mon, 19 Jul 2021 21:18:42 GMT, Joe Darcy wrote: > Given changes in JLS 9.6.4.1, JDK-8261610 in Java SE 17, the statements about > annotation applicability made in java.lang.annotation.Target need to be > updated to match. > > Please also review the corresponding CSR: > https://bugs.openjdk.java.net/browse/JDK-8270917 Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/256
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes
On Mon, 28 Jun 2021 20:50:42 GMT, Ian Graves wrote: > 8199594: Add doc describing how (?x) ignores spaces in character classes Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8260265: UTF-8 by Default [v4]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed leftover `console` references in `PrintStream` - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/182dcb6d..38b8d988 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=02-03 Stats: 14 lines in 1 file changed: 0 ins; 4 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v3]
On Wed, 14 Jul 2021 20:52:54 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/io/Console.java line 587:
>>
>>> 585: try {
>>> 586: cs = Charset.forName(csname);
>>> 587: } catch (Exception ignored) { }
>>
>> A separate enhancement...
>> I've long thought that should be a way to avoid the exception here.
>> For example, a Charset.forName(csname, default);
>> The caller might have a default in mind or supply null and then be able to
>> test for null.
>
> Agreed. Will file an RFE for this.
https://bugs.openjdk.java.net/browse/JDK-8270490
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v3]
On Wed, 14 Jul 2021 21:03:53 GMT, Roger Riggs wrote: >> That was intentional. Only those two are supported, others continue to work >> as before (but not supported). > > Still it leaves an uncomfortable feeling, perhaps remedied by an "other > values have unspecified behavior" > or the "other values are implementation specific". Added the clarifying sentence to the spec. - PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v3]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/981200f7..182dcb6d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=01-02 Stats: 28 lines in 2 files changed: 0 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
On Wed, 14 Jul 2021 16:20:28 GMT, Jesse Glick
wrote:
>> src/java.base/share/classes/java/io/PrintStream.java line 49:
>>
>>> 47: * All characters printed by a {@code PrintStream} are converted
>>> into
>>> 48: * bytes using the given encoding or charset, or the default
>>> 49: * console charset if not specified.
>>
>> JEP 400 doesn't give a rationale for using the console charset for
>> PrintStream.
>> PrintStreams are used for output to files and other media other than just a
>> tty/console.
>> The charset of system.out/err should use the console charset.
>
> This was my thinking in
> https://github.com/openjdk/jdk/pull/4733#issuecomment-876793372.
OK, I am now conviced. Modified not to default to Console.charset() for generic
PrintStream w/o charset constructor.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato 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 three additional commits since the last revision: - Reflects review comments - Merge branch 'master' into JDK-8260265 - 8260265: UTF-8 by Default - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/107210cf..981200f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=00-01 Stats: 6434 lines in 314 files changed: 3877 ins; 1452 del; 1105 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
On Wed, 14 Jul 2021 14:55:39 GMT, Roger Riggs wrote:
>> Naoto Sato 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 three additional
>> commits since the last revision:
>>
>> - Reflects review comments
>> - Merge branch 'master' into JDK-8260265
>> - 8260265: UTF-8 by Default
>
> src/java.base/share/classes/java/io/Console.java line 587:
>
>> 585: try {
>> 586: cs = Charset.forName(csname);
>> 587: } catch (Exception ignored) { }
>
> A separate enhancement...
> I've long thought that should be a way to avoid the exception here.
> For example, a Charset.forName(csname, default);
> The caller might have a default in mind or supply null and then be able to
> test for null.
Agreed. Will file an RFE for this.
> src/java.base/share/classes/java/io/FileReader.java line 41:
>
>> 39: * @see InputStreamReader
>> 40: * @see FileInputStream
>> 41: * @see java.nio.charset.Charset#defaultCharset()
>
> The @ see duplicates the link above, the javadoc can do without the @ see.
If I remove that `@see`, I don't see the link in `See Also` section. Am I
missing something?
> src/java.base/share/classes/java/lang/System.java line 802:
>
>> 800: * {@systemProperty file.encoding}
>> 801: * The name of the default charset. Users may specify
>> 802: * {@code UTF-8} or {@code COMPAT} on the command line to the
>> value.
>
> The wording could imply that only those two values can be supplied.
> It could be rephrased to say that *if* the property is supplied on the
> command line
> it overrides the default UTF-8.
That was intentional. Only those two are supported, others continue to work as
before (but not supported).
> src/java.base/share/classes/java/nio/charset/Charset.java line 601:
>
>> 599: * value designates {@code COMPAT}, the default charset is derived
>> from
>> 600: * the {@code native.encoding} system property, which typically
>> depends
>> 601: * upon the locale and charset of the underlying operating system.
>
> The description in java.lang.System of the file.encoding property does not
> indicate it is 'implementation specific'.
> In that context, it appears to be part of the JavaSE spec.
> Having the spec in a single place with references to it from others could
> avoid duplication.
`file.encoding` is listed under `@implNote` tag in `System.getProperties()`, so
it is implementation-specific.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default
On Wed, 14 Jul 2021 12:39:46 GMT, Giacomo Baso
wrote:
> > Consider an application that creates a java.io.FileWriter with its
> > one-argument constructor and then uses it to write some text to a file. The
> > resulting file will contain a sequence of bytes encoded using the default
> > charset of the JDK running the application. A second application, run on a
> > different machine or by a different user on the same machine, creates a
> > java.io.FileReader with its one-argument constructor and uses it to read
> > the bytes in that file. The resulting text contains a sequence of
> > characters decoded using the default charset of the JDK running the second
> > application. If the default charset differs between the JDK of the first
> > application and the JDK of the second application, then the resulting text
> > may be silently corrupted or incomplete, since these APIs replace erroneous
> > input rather than fail.
>
> It's even worse than that, because many OpenSSH installs are configured by
> default to [forward](https://man.openbsd.org/ssh_config.5#SendEnv) and
> [accept](https://man.openbsd.org/sshd_config.5#AcceptEnv) the user locale
> (see e.g. for [RHEL 7](https://access.redhat.com/solutions/974273)).
>
> So a single application, on a single remote machine, can be unknowingly
> started by a single user with different locales, and therefore different
> encodings, depending on how the user connected to the remote machine. For
> example, on Windows connecting via powershell results in `LANG=en_US.UTF-8`,
> while using WSL2 results in `LANG=C.UTF-8`. On Java 11 in a RHEL7 machine,
> `file.encoding` results in `UTF-8` in the first case, but `ANSI_X3.4-1968` in
> the second, leading to a default charset `ASCII`.
>
> Worth mentioning is also that `Charset.forName("default")` is just an alias
> to `ASCII`, per `sun.nio.cs.StandardCharsets$Aliases`.
Thanks. Updated the JEP.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
RFR: 8260265: UTF-8 by Default
This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of the changes is `Charset.defaultCharset()` returning `UTF-8` and `file.encoding` system property being added in the spec, but another notable modification is in `java.io.PrintStream` where it continues to use the `Console` encoding as the default charset instead of `UTF-8`. Other changes are mostly clarification of the term "default charset" and their links. Corresponding CSR has also been drafted. JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 - Commit messages: - 8260265: UTF-8 by Default Changes: https://git.openjdk.java.net/jdk/pull/4733/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4733=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260265 Stats: 297 lines in 18 files changed: 184 ins; 9 del; 104 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: [jdk17] RFR: 8269929: (test) Add diagnostic info to ProceessBuilder/Basic.java for unexpected output
On Wed, 7 Jul 2021 19:05:14 GMT, Roger Riggs wrote: > The test java/lang/ProcessBuilder/Basic.java continues to fail intermittently > with unexpected output from the VM. > It appears that destroying the process causes a vm thread to fail to be > started. > Extend the delay between starting the child and destroying it. > Add diagnostics to be specific about which case failed. > Incidentally, suppress compiler warnings about SecurityManager use. Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/228
Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]
On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter wrote: >> Modify the specification of >> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is >> returned instead of `0` when the stream is at its end and the third >> parameter, `len`, is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6766844: Correct error messages in test Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/189
Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]
On Fri, 2 Jul 2021 16:55:18 GMT, Brian Burkhalter wrote:
>> Modify the specification of
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is
>> returned instead of `0` when the stream is at its end and the third
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 6766844: Correct error messages in test
test/jdk/java/io/ByteArrayInputStream/ReadAllReadNTransferTo.java line 57:
> 55: }
> 56: if (bais.read(new byte[1], 0, 0) != -1) {
> 57: throw new RuntimeException("read(byte[],int,int) did not
> return 0");
Should these exception messages be "did not return -1"?
-
PR: https://git.openjdk.java.net/jdk17/pull/189
[jdk17] Integrated: 8269704: Typo in j.t.Normalizer.normalize()
On Wed, 30 Jun 2021 21:38:43 GMT, Naoto Sato wrote: > A trivial typo fix. This pull request has now been integrated. Changeset: 54dd510b Author: Naoto Sato URL: https://git.openjdk.java.net/jdk17/commit/54dd510bd5211dc440285dd53ca0e41c85e23552 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8269704: Typo in j.t.Normalizer.normalize() Reviewed-by: joehw, prappo, iris - PR: https://git.openjdk.java.net/jdk17/pull/187
[jdk17] Integrated: 8269513: Clarify the spec wrt `useOldISOCodes` system property
On Mon, 28 Jun 2021 16:57:15 GMT, Naoto Sato wrote: > Please review this small doc change to the system property. Accompanying CSR > has also been created. This pull request has now been integrated. Changeset: 3e022247 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk17/commit/3e022247d2e80c43393bfdb5888b03210c6975d3 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8269513: Clarify the spec wrt `useOldISOCodes` system property Reviewed-by: lancea, bpb, iris, joehw - PR: https://git.openjdk.java.net/jdk17/pull/163
