Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v5]

[Naoto Sato]

On Fri, 16 Apr 2021 04:06:54 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>> 
>> As noted in a comment in that issue, the bug relates to the return value of 
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The 
>> implementation has started returning invalid values for the `AM_PM` field 
>> after the "day period" support was added recently in the JDK as part of 
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>> 
>> The commit here adds a check in the internal implementation of the display 
>> name handling logic, to special case the `AM_PM` field and properly convert 
>> the display name array indexes (which is an internal detail) to valid values 
>> that represent the `AM_PM` calendar field.
>> 
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
>> reproduces this issue and verifies the fix.
>> 
>> After this fix was introduced, I ran the test in 
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
>> issue in the first place? To fix this, I have added an additional commit 
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update existing test based on review comments

Looks good. Thank you for fixing the issue.

-

Marked as reviewed by naoto (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v5]

[Jaikiran Pai]

> Can I please get a review for this proposed fix for 
> https://bugs.openjdk.java.net/browse/JDK-8262108?
> 
> As noted in a comment in that issue, the bug relates to the return value of 
> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation 
> has started returning invalid values for the `AM_PM` field after the "day 
> period" support was added recently in the JDK as part of 
> https://bugs.openjdk.java.net/browse/JDK-8262108.
> 
> The commit here adds a check in the internal implementation of the display 
> name handling logic, to special case the `AM_PM` field and properly convert 
> the display name array indexes (which is an internal detail) to valid values 
> that represent the `AM_PM` calendar field.
> 
> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
> reproduces this issue and verifies the fix.
> 
> After this fix was introduced, I ran the test in 
> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
> issue in the first place? To fix this, I have added an additional commit 
> which updates this test case to properly test the `AM_PM` field values.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  Update existing test based on review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3463/files
  - new: https://git.openjdk.java.net/jdk/pull/3463/files/607776b3..ea0d3c77

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3463=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3463=03-04

  Stats: 49 lines in 1 file changed: 0 ins; 45 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3463.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]

[Jaikiran Pai]

On Fri, 16 Apr 2021 03:51:22 GMT, Naoto Sato  wrote:

>> Hello Naoto,
>> 
>> As far as I can see, the testMap cannot be used to test the day period 
>> strings. More specifically, consider this change (git diff) that I did as 
>> you suggested (unless I misunderstood what you meant):
>> 
>> 
>> +testMap(US, AM_PM, ALL_STYLES,
>> +"AM",
>> +"PM",
>> +"midnight",
>> +"noon",
>> +"in the morning",
>> +"",
>> +"in the afternoon",
>> +"",
>> +"in the evening",
>> +"",
>> +"at night",
>> +"",
>> +RESET_INDEX,
>> +"a", "p", "mi", "n", "", "", "", "", "", "", "", "");
>> +testMap(US, AM_PM, ALL_STYLES,
>> +"AM", "PM",
>> +RESET_INDEX,
>> +"a", "p");
>> 
>> 
>> 
>> This will fail with the following error.
>> 
>> testMap: locale=en_US, field=9, style=0, expected={in the afternoon=6, in 
>> the evening=8, in the morning=4, at night=10, midnight=2, noon=3, AM=0, 
>> PM=1, mi=2, a=0, n=3, p=1}, got={AM=0, PM=1, a=0, p=1}
>> java.lang.RuntimeException: test failed
>> 
>> 
>> That failure is due to the `testMap` method expecting these day period 
>> string to be part of the returned Map from `Calendar.getDisplayNames()` 
>> which won't be the case because that API (as per the javadoc) will only 
>> return a Map whose display names have valid field values, so in this case 
>> only those display names which will have `AM` or `PM` as a value.
>> 
>> If it's easier to review, I will go ahead and push the suggested change in 
>> this testcase and let if fail so that you can get a chance to look at the 
>> actual test code and error. Let me know.
>
> Sorry if I wasn't clear. The whole test for am/pm (line 98 - 118) can simply 
> be:
> 
> testMap(US, AM_PM, ALL_STYLES,
> "AM", "PM",
> RESET_INDEX,
> "a", "p");
> 
> Since we now know that day periods strings won't be leaked into display 
> names, then the map simply should contain only 4 entries (`AM`, `PM`, `a`, 
> and `p`).

Thank you for that clarification. I now understand what you mean. I've updated 
the test to follow this suggestion in the latest update to this PR.

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]

[Naoto Sato]

On Fri, 16 Apr 2021 02:35:23 GMT, Jaikiran Pai  wrote:

>> test/jdk/java/util/Calendar/NarrowNamesTest.java line 115:
>> 
>>> 113: } else {
>>> 114: testMap(US, AM_PM, ALL_STYLES,
>>> 115: "AM", "PM",
>> 
>> What I meant was there is no need to check the providers and introduce the 
>> am/pm specific test method. Both `CLDR` and `COMPAT` (i.e., no "if" 
>> statement) should be tested with `testMap()` method as other tests do.
>
> Hello Naoto,
> 
> As far as I can see, the testMap cannot be used to test the day period 
> strings. More specifically, consider this change (git diff) that I did as you 
> suggested (unless I misunderstood what you meant):
> 
> 
> +testMap(US, AM_PM, ALL_STYLES,
> +"AM",
> +"PM",
> +"midnight",
> +"noon",
> +"in the morning",
> +"",
> +"in the afternoon",
> +"",
> +"in the evening",
> +"",
> +"at night",
> +"",
> +RESET_INDEX,
> +"a", "p", "mi", "n", "", "", "", "", "", "", "", "");
> +testMap(US, AM_PM, ALL_STYLES,
> +"AM", "PM",
> +RESET_INDEX,
> +"a", "p");
> 
> 
> 
> This will fail with the following error.
> 
> testMap: locale=en_US, field=9, style=0, expected={in the afternoon=6, in the 
> evening=8, in the morning=4, at night=10, midnight=2, noon=3, AM=0, PM=1, 
> mi=2, a=0, n=3, p=1}, got={AM=0, PM=1, a=0, p=1}
> java.lang.RuntimeException: test failed
> 
> 
> That failure is due to the `testMap` method expecting these day period string 
> to be part of the returned Map from `Calendar.getDisplayNames()` which won't 
> be the case because that API (as per the javadoc) will only return a Map 
> whose display names have valid field values, so in this case only those 
> display names which will have `AM` or `PM` as a value.
> 
> If it's easier to review, I will go ahead and push the suggested change in 
> this testcase and let if fail so that you can get a chance to look at the 
> actual test code and error. Let me know.

Sorry if I wasn't clear. The whole test for am/pm (line 98 - 118) can simply be:

testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
RESET_INDEX,
"a", "p");

Since we now know that day periods strings won't be leaked into display names, 
then the map simply should contain only 4 entries (`AM`, `PM`, `a`, and `p`).

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v3]

[Vicente Romero]

> Please review this PR that intents to make sealed classes a final feature in 
> Java. This PR contains compiler and VM changes. In line with similar PRs, 
> which has made preview features final, this one is mostly removing preview 
> related comments from APIs plus options in test cases. Please also review the 
> related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090)
> 
> Thanks
> 
> note: this PR is related to 
> [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated 
> after it.

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  updating comment after review feedback

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3526/files
  - new: https://git.openjdk.java.net/jdk/pull/3526/files/5aef5108..8ebe56fd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3526.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526

PR: https://git.openjdk.java.net/jdk/pull/3526

Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v2]

[Vicente Romero]

On Fri, 16 Apr 2021 02:10:05 GMT, David Holmes  wrote:

> Hi Vicente,
> 
> Hotspot and hotspot tests all look fine. One query: why was this test removed?
> 
> test/hotspot/jtreg/runtime/sealedClasses/AbstractSealedTest.java
> 
> is that functionality tested elsewhere? (The other deleted test seemed 
> obviously trivial.)
> 
> Thanks,
> David

Hi David, thanks for your comments, yes regarding `test 
test/hotspot/jtreg/runtime/sealedClasses/AbstractSealedTest.java`, it was 
removed because the functionality is tested in 
`test/langtools/tools/javac/sealed/SealedCompilationTests.java`

> src/hotspot/share/classfile/classFileParser.cpp line 3916:
> 
>> 3914: record_attribute_start = cfs->current();
>> 3915: record_attribute_length = attribute_length;
>> 3916:   } else if (_major_version >= JAVA_17_VERSION) {
> 
> Can you update the comment at L3932 to say JAVA_17_VERSION please.

sure

-

PR: https://git.openjdk.java.net/jdk/pull/3526

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]

[Jaikiran Pai]

On Thu, 15 Apr 2021 03:57:18 GMT, Naoto Sato  wrote:

>> Jaikiran Pai has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - update existing testcase based on review comment
>>  - Improve code comment to be clear it's only applicable for 
>> java.util.Calendar
>>  - Remove irrelevant setLenient from new testcase
>
> test/jdk/java/util/Calendar/NarrowNamesTest.java line 115:
> 
>> 113: } else {
>> 114: testMap(US, AM_PM, ALL_STYLES,
>> 115: "AM", "PM",
> 
> What I meant was there is no need to check the providers and introduce the 
> am/pm specific test method. Both `CLDR` and `COMPAT` (i.e., no "if" 
> statement) should be tested with `testMap()` method as other tests do.

Hello Naoto,

As far as I can see, the testMap cannot be used to test the day period strings. 
More specifically, consider this change (git diff) that I did as you suggested 
(unless I misunderstood what you meant):


+testMap(US, AM_PM, ALL_STYLES,
+"AM",
+"PM",
+"midnight",
+"noon",
+"in the morning",
+"",
+"in the afternoon",
+"",
+"in the evening",
+"",
+"at night",
+"",
+RESET_INDEX,
+"a", "p", "mi", "n", "", "", "", "", "", "", "", "");
+testMap(US, AM_PM, ALL_STYLES,
+"AM", "PM",
+RESET_INDEX,
+"a", "p");



This will fail with the following error.

testMap: locale=en_US, field=9, style=0, expected={in the afternoon=6, in the 
evening=8, in the morning=4, at night=10, midnight=2, noon=3, AM=0, PM=1, mi=2, 
a=0, n=3, p=1}, got={AM=0, PM=1, a=0, p=1}
java.lang.RuntimeException: test failed


That failure is due to the `testMap` method expecting these day period string 
to be part of the returned Map from `Calendar.getDisplayNames()` which won't be 
the case because that API (as per the javadoc) will only return a Map whose 
display names have valid field values, so in this case only those display names 
which will have `AM` or `PM` as a value.

If it's easier to review, I will go ahead and push the suggested change in this 
testcase and let if fail so that you can get a chance to look at the actual 
test code and error. Let me know.

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v2]

[Vicente Romero]

> Please review this PR that intents to make sealed classes a final feature in 
> Java. This PR contains compiler and VM changes. In line with similar PRs, 
> which has made preview features final, this one is mostly removing preview 
> related comments from APIs plus options in test cases. Please also review the 
> related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090)
> 
> Thanks
> 
> note: this PR is related to 
> [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated 
> after it.

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  removing javax.lang.model changes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3526/files
  - new: https://git.openjdk.java.net/jdk/pull/3526/files/6e2a99c6..5aef5108

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=00-01

  Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3526.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526

PR: https://git.openjdk.java.net/jdk/pull/3526

Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v2]

[David Holmes]

On Fri, 16 Apr 2021 02:11:10 GMT, Vicente Romero  wrote:

>> Please review this PR that intents to make sealed classes a final feature in 
>> Java. This PR contains compiler and VM changes. In line with similar PRs, 
>> which has made preview features final, this one is mostly removing preview 
>> related comments from APIs plus options in test cases. Please also review 
>> the related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090)
>> 
>> Thanks
>> 
>> note: this PR is related to 
>> [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated 
>> after it.
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   removing javax.lang.model changes

Hi Vincente,

Hotspot and hotspot tests all look fine. One query: why was this test removed?

 test/hotspot/jtreg/runtime/sealedClasses/AbstractSealedTest.java

is that functionality tested elsewhere? (The other deleted test seemed 
obviously trivial.)

Thanks,
David

src/hotspot/share/classfile/classFileParser.cpp line 3916:

> 3914: record_attribute_start = cfs->current();
> 3915: record_attribute_length = attribute_length;
> 3916:   } else if (_major_version >= JAVA_17_VERSION) {

Can you update the comment at L3932 to say JAVA_17_VERSION please.

src/hotspot/share/classfile/classFileParser.hpp line 345:

> 343: 
> 344:   bool supports_sealed_types();
> 345:   bool supports_records();

Good catch!

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3526

RFR: 8260517: implement Sealed Classes as a standard feature

[Vicente Romero]

Please review this PR that intents to make sealed classes a final feature in 
Java. This PR contains compiler and VM changes. In line with similar PRs, which 
has made preview features final, this one is mostly removing preview related 
comments from APIs plus options in test cases.

Thanks

-

Commit messages:
 - Merge branch 'master' into JDK-8260517
 - Merge branch 'master' into JDK-8260517
 - fixing failing regression tests
 - JVM related changes
 - 8260517: Compiler implementation for Sealed Classes

Changes: https://git.openjdk.java.net/jdk/pull/3526/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3526=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260517
  Stats: 450 lines in 56 files changed: 48 ins; 282 del; 120 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3526.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526

PR: https://git.openjdk.java.net/jdk/pull/3526

Re: RFR: 8263154: [macos] DMG builds have finder errors

[Alexander Matveev]

On Thu, 15 Apr 2021 01:24:04 GMT, Alexander Matveev  
wrote:

> - Issue was reproducible when install-dir points to some invalid location.
>  - Fixed by defaulting DMG drag and drop location to /Applications folder and 
> --install-dir will be ignored with warning for DMG.
>  - I do not see any way to support other valid, but uncommon locations for 
> drag and drop. For example: /Users/USERNAME/Applications is not possible to 
> support since user name is not known. /usr/bin requires root privileges and 
> should contain symbolic links. Locations which does not exist also not 
> possible to support, since DMG cannot create paths. So 
> /Applications/MyCompany is not possible for DMG.

Not sure if I understood question about PKG. PKG can create destination folders 
and will have root access once elevated, so user can specify any locations for 
--install-dir in case of PKG.
Well DMG is not an installer it is just disk image which contains app (think of 
it as archive file), so we cannot really support all functionality with DMG as 
with actual installers.
Yes, I can check if install-dir exist with apple script, but destination 
machine might not have this folder. Permissions might be different on folder on 
machine which generates DMG and machine which will install it.
If PKG cannot be install into user home Applications folder, then it is another 
bug.

Also, if someone really want --install-dir to some other location they can 
overwrite apple script we using via resource folder. This fix only affects 
apple script we generate to customize DMG. Extra step for user, but I think it 
will not be common anyway.

-

PR: https://git.openjdk.java.net/jdk/pull/3505

Integrated: 8258794: Support for CLDR version 39

[Naoto Sato]

On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato  wrote:

> Please review the changes to support CLDR version 39. The vast majority of 
> the changes are purely data changes from Unicode. The only change affected in 
> logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with 
> CLDR's Norwegian language code switch 
> (https://unicode-org.atlassian.net/browse/CLDR-2698)

This pull request has now been integrated.

Changeset: f6e54f2f
Author:Naoto Sato 
URL:   https://git.openjdk.java.net/jdk/commit/f6e54f2f
Stats: 26326 lines in 815 files changed: 761 ins; 23140 del; 2425 mod

8258794: Support for CLDR version 39

Reviewed-by: joehw, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/3502

Integrated: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it

[Brian Burkhalter]

On Wed, 7 Apr 2021 21:01:48 GMT, Brian Burkhalter  wrote:

> The specification of the method `flush()` in the `java.io` classes 
> `CharArrayWriter` and `StringWriter` is not explicit about the fact that the 
> method has no effect. This request proposes to add to the specification of 
> each flush() method the sentence
> 
> The {@code flush} method of {@code } does nothing.
> 
> The corresponding CSR is JDK-8264867.

This pull request has now been integrated.

Changeset: e89fd151
Author:Brian Burkhalter 
URL:   https://git.openjdk.java.net/jdk/commit/e89fd151
Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod

8261301: StringWriter.flush() is NOOP but documentation does not indicate it

Reviewed-by: naoto, rriggs

-

PR: https://git.openjdk.java.net/jdk/pull/3383

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Peter Levart]

On Wed, 14 Apr 2021 20:03:27 GMT, Claes Redestad  wrote:

> There's a StringJoinerBenchmark micro added by JDK-8148937 which could 
> perhaps be expanded with the scenarios you've experimented with here?

I modified that micro benchmark and added a method to also measure String.join 
static method along with StringJoiner for same parameters and extended the 
range of parameters to cover more diversity. The results are here:

https://jmh.morethan.io/?gist=c38cc13d63774ec505cc8d394c00d502

It is apparent that there is a huge speedup when strings are bigger. But even 
smaller strings get a substantial speedup. There's also substantial reduction 
of garbage per operation. Previously the garbage amounted to the internal array 
of String elements and the StringBuffer with its internal byte[] array of 
characters. Now only the array of elements is the garbage.

-

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: RFR: 8265237: String.join and StringJoiner can be improved further [v2]

[Peter Levart]

> While JDK-8148937 improved StringJoiner class by replacing internal use of 
> getChars that copies out characters from String elements into a char[] array 
> with StringBuilder which is somehow more optimal, the improvement was 
> marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was 
> reduced by about 50% in average per operation.
> Initial attempt to tackle that issue was more involved, but was later 
> discarded because it was apparently using too much internal String details in 
> code that lives outside String and outside java.lang package.
> But there is another way to package such "intimate" code - we can put it into 
> String itself and just call it from StringJoiner.
> This PR is an attempt at doing just that. It introduces new package-private 
> method in `java.lang.String` which is then used from both pubic static 
> `String.join` methods as well as from `java.util.StringJoiner` (via 
> SharedSecrets). The improvements can be seen by running the following JMH 
> benchmark:
> 
> https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce
> 
> The comparative results are here:
> 
> https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15
> 
> The jmh-result.json files are here:
> 
> https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15
> 
> Improvement in speed ranges from 8% (for small strings) to 200% (for long 
> strings), while creation of garbage has been further reduced to an almost 
> garbage-free operation.
> 
> So WDYT?

Peter Levart has updated the pull request incrementally with one additional 
commit since the last revision:

  Add String.join benchmark method to StringJoinerBenchmark and adjust some 
parameters to cover bigger range

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3501/files
  - new: https://git.openjdk.java.net/jdk/pull/3501/files/62b577fd..6160e5aa

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3501=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3501=00-01

  Stats: 11 lines in 1 file changed: 8 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3501.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3501/head:pull/3501

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Joe Darcy]

Hi Jan,

I recommend using {Double, Float}.toHexString to get a straightforward 
textual form of the floating-point values. The hex string is isomorphic 
to the big-level value, but is (more) human readable as a numerical 
quantity.


-Joe

On 4/15/2021 10:26 AM, Jan Lahoda wrote:

On Thu, 8 Apr 2021 21:12:21 GMT, Raffaello Giulietti 
 wrote:


Hello,

here's a PR for a patch submitted on March 2020 
[1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a 
thing.

The patch has been edited to adhere to OpenJDK code conventions about 
multi-line (block) comments. Nothing in the code proper has changed, except for 
the addition of redundant but clarifying parentheses in some expressions.


Greetings
Raffaello

Regarding the `ElementStructureTest`, it prints the API elements (including 
compile-time constants) computes hash for the printed API and compares it with 
an expected hash. doubles and floats are printed using String.valueOf, and it 
apparently changed for `Float.MIN_NORMAL` from `1.17549435E-38` to 
`1.1754944E-38` (I assume that is intentional). So regarding 
`ElementStructureTest.java` we can just update it. How about this?


diff --git a/test/langtools/tools/javac/sym/ElementStructureTest.java 
b/test/langtools/tools/javac/sym/ElementStructureTest.java
index 29776ce28c2..d15f2447749 100644
--- a/test/langtools/tools/javac/sym/ElementStructureTest.java
+++ b/test/langtools/tools/javac/sym/ElementStructureTest.java
@@ -121,29 +121,22 @@ import toolbox.ToolBox;
   */
  public class ElementStructureTest {
  
-static final byte[] hash6 = new byte[] {

-(byte) 0x99, (byte) 0x34, (byte) 0x82, (byte) 0xCF,
-(byte) 0xE0, (byte) 0x53, (byte) 0xF3, (byte) 0x13,
-(byte) 0x4E, (byte) 0xCF, (byte) 0x49, (byte) 0x32,
-(byte) 0xB7, (byte) 0x52, (byte) 0x0F, (byte) 0x68
-};
  static final byte[] hash7 = new byte[] {
-(byte) 0x3C, (byte) 0x03, (byte) 0xEA, (byte) 0x4A,
-(byte) 0x62, (byte) 0xD2, (byte) 0x18, (byte) 0xE5,
-(byte) 0xA5, (byte) 0xC2, (byte) 0xB7, (byte) 0x85,
-(byte) 0x90, (byte) 0xFA, (byte) 0x98, (byte) 0xCD
+(byte) 0xA7, (byte) 0x3B, (byte) 0x91, (byte) 0xF6,
+(byte) 0xEF, (byte) 0x99, (byte) 0x07, (byte) 0xF2,
+(byte) 0x79, (byte) 0xAB, (byte) 0x19, (byte) 0xF4,
+(byte) 0x59, (byte) 0x44, (byte) 0xF7, (byte) 0x65
  };
  static final byte[] hash8 = new byte[] {
-(byte) 0x24, (byte) 0x38, (byte) 0x52, (byte) 0x1C,
-(byte) 0x5E, (byte) 0x83, (byte) 0x82, (byte) 0xE6,
-(byte) 0x41, (byte) 0xC2, (byte) 0xDD, (byte) 0x2A,
-(byte) 0xFD, (byte) 0xFF, (byte) 0x5E, (byte) 0x2F
+(byte) 0xF3, (byte) 0x93, (byte) 0xCA, (byte) 0x53,
+(byte) 0xFD, (byte) 0xA3, (byte) 0x5D, (byte) 0x57,
+(byte) 0xD2, (byte) 0xED, (byte) 0x39, (byte) 0xC5,
+(byte) 0x56, (byte) 0x62, (byte) 0xE0, (byte) 0x1F
  };
  
  final static Map version2Hash = new HashMap<>();
  
  static {

-version2Hash.put("6", hash6);
  version2Hash.put("7", hash7);
  version2Hash.put("8", hash8);
  }
@@ -484,7 +477,7 @@ public class ElementStructureTest {
  return null;
  try {
  analyzeElement(e);
-out.write(String.valueOf(e.getConstantValue()));
+writeConstant(e.getConstantValue());
  out.write("\n");
  } catch (IOException ex) {
  ex.printStackTrace();
@@ -514,6 +507,16 @@ public class ElementStructureTest {
  throw new IllegalStateException("Should not get here.");
  }
  
+private void writeConstant(Object value) throws IOException {

+if (value instanceof Double) {
+out.write(Long.toString(Double.doubleToRawLongBits((Double) 
value)));
+} else if (value instanceof Float) {
+out.write(Integer.toString(Float.floatToRawIntBits((Float) 
value)));
+} else {
+out.write(String.valueOf(value));
+}
+}
+
  }
  
  final class TestFileManager implements JavaFileManager {


-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 8264208: Console charset API [v8]

[Naoto Sato]

On Thu, 15 Apr 2021 14:17:11 GMT, Alan Bateman  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added @see links.
>
> src/java.base/share/classes/java/io/Console.java line 397:
> 
>> 395: /**
>> 396:  * Returns the {@link java.nio.charset.Charset Charset} object used 
>> in
>> 397:  * the {@code Console}.
> 
> What would you think about re-phrasing the first sentence to use "for the 
> Console" rather than "in the Console".

Changed to "for the Console", as well as `@return`.

> src/java.base/share/classes/java/lang/System.java line 123:
> 
>> 121:  *
>> 122:  * @see Console#charset()
>> 123:  * @see Console#reader()
> 
> What would you think about changing the example in InputStreamReader class 
> description as part of this?

Replaced `System.in` with generic `anInputStream`, as changing `new 
InputStreamReader` with `Console.reader()` would defy the purpose of the 
example.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264208: Console charset API [v9]

[Naoto Sato]

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Modified javadocs per suggestions.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3419/files
  - new: https://git.openjdk.java.net/jdk/pull/3419/files/5988f600..083f6180

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=07-08

  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results

[Jan Lahoda]

On Thu, 8 Apr 2021 21:12:21 GMT, Raffaello Giulietti 
 wrote:

> Hello,
> 
> here's a PR for a patch submitted on March 2020 
> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a 
> thing.
> 
> The patch has been edited to adhere to OpenJDK code conventions about 
> multi-line (block) comments. Nothing in the code proper has changed, except 
> for the addition of redundant but clarifying parentheses in some expressions.
> 
> 
> Greetings
> Raffaello

Regarding the `ElementStructureTest`, it prints the API elements (including 
compile-time constants) computes hash for the printed API and compares it with 
an expected hash. doubles and floats are printed using String.valueOf, and it 
apparently changed for `Float.MIN_NORMAL` from `1.17549435E-38` to 
`1.1754944E-38` (I assume that is intentional). So regarding 
`ElementStructureTest.java` we can just update it. How about this?


diff --git a/test/langtools/tools/javac/sym/ElementStructureTest.java 
b/test/langtools/tools/javac/sym/ElementStructureTest.java
index 29776ce28c2..d15f2447749 100644
--- a/test/langtools/tools/javac/sym/ElementStructureTest.java
+++ b/test/langtools/tools/javac/sym/ElementStructureTest.java
@@ -121,29 +121,22 @@ import toolbox.ToolBox;
  */
 public class ElementStructureTest {
 
-static final byte[] hash6 = new byte[] {
-(byte) 0x99, (byte) 0x34, (byte) 0x82, (byte) 0xCF,
-(byte) 0xE0, (byte) 0x53, (byte) 0xF3, (byte) 0x13,
-(byte) 0x4E, (byte) 0xCF, (byte) 0x49, (byte) 0x32,
-(byte) 0xB7, (byte) 0x52, (byte) 0x0F, (byte) 0x68
-};
 static final byte[] hash7 = new byte[] {
-(byte) 0x3C, (byte) 0x03, (byte) 0xEA, (byte) 0x4A,
-(byte) 0x62, (byte) 0xD2, (byte) 0x18, (byte) 0xE5,
-(byte) 0xA5, (byte) 0xC2, (byte) 0xB7, (byte) 0x85,
-(byte) 0x90, (byte) 0xFA, (byte) 0x98, (byte) 0xCD
+(byte) 0xA7, (byte) 0x3B, (byte) 0x91, (byte) 0xF6,
+(byte) 0xEF, (byte) 0x99, (byte) 0x07, (byte) 0xF2,
+(byte) 0x79, (byte) 0xAB, (byte) 0x19, (byte) 0xF4,
+(byte) 0x59, (byte) 0x44, (byte) 0xF7, (byte) 0x65
 };
 static final byte[] hash8 = new byte[] {
-(byte) 0x24, (byte) 0x38, (byte) 0x52, (byte) 0x1C,
-(byte) 0x5E, (byte) 0x83, (byte) 0x82, (byte) 0xE6,
-(byte) 0x41, (byte) 0xC2, (byte) 0xDD, (byte) 0x2A,
-(byte) 0xFD, (byte) 0xFF, (byte) 0x5E, (byte) 0x2F
+(byte) 0xF3, (byte) 0x93, (byte) 0xCA, (byte) 0x53,
+(byte) 0xFD, (byte) 0xA3, (byte) 0x5D, (byte) 0x57,
+(byte) 0xD2, (byte) 0xED, (byte) 0x39, (byte) 0xC5,
+(byte) 0x56, (byte) 0x62, (byte) 0xE0, (byte) 0x1F
 };
 
 final static Map version2Hash = new HashMap<>();
 
 static {
-version2Hash.put("6", hash6);
 version2Hash.put("7", hash7);
 version2Hash.put("8", hash8);
 }
@@ -484,7 +477,7 @@ public class ElementStructureTest {
 return null;
 try {
 analyzeElement(e);
-out.write(String.valueOf(e.getConstantValue()));
+writeConstant(e.getConstantValue());
 out.write("\n");
 } catch (IOException ex) {
 ex.printStackTrace();
@@ -514,6 +507,16 @@ public class ElementStructureTest {
 throw new IllegalStateException("Should not get here.");
 }
 
+private void writeConstant(Object value) throws IOException {
+if (value instanceof Double) {
+out.write(Long.toString(Double.doubleToRawLongBits((Double) 
value)));
+} else if (value instanceof Float) {
+out.write(Integer.toString(Float.floatToRawIntBits((Float) 
value)));
+} else {
+out.write(String.valueOf(value));
+}
+}
+
 }
 
 final class TestFileManager implements JavaFileManager {

-

PR: https://git.openjdk.java.net/jdk/pull/3402

Re: RFR: 8263154: [macos] DMG builds have finder errors

[Andy Herrick]

On Thu, 15 Apr 2021 01:24:04 GMT, Alexander Matveev  
wrote:

> - Issue was reproducible when install-dir points to some invalid location.
>  - Fixed by defaulting DMG drag and drop location to /Applications folder and 
> --install-dir will be ignored with warning for DMG.
>  - I do not see any way to support other valid, but uncommon locations for 
> drag and drop. For example: /Users/USERNAME/Applications is not possible to 
> support since user name is not known. /usr/bin requires root privileges and 
> should contain symbolic links. Locations which does not exist also not 
> possible to support, since DMG cannot create paths. So 
> /Applications/MyCompany is not possible for DMG.

Doesn't much of this argument apply to pkg builds too ?
I hate to abandon install-dir option entirely for dmg, and just printing a 
warning and ignoring the argument is contrary to what is done for other 
arguments.
Is there any way, in the apple script, to check if the install-dir exists, and 
to use it if it does ?
Also wouldn't it be useful (for both pkg and dmg) to have ability to install 
into the users home directory ?
just specifying ~ or ~/Applications gets expanded before going in script if not 
quoted, and doesn't work if it is quoted.
Unless we define a meaning for non-fully qualified path in install-dir that 
should probably be a hard error..

-

PR: https://git.openjdk.java.net/jdk/pull/3505

Integrated: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test

[Roger Riggs]

On Wed, 14 Apr 2021 14:08:50 GMT, Roger Riggs  wrote:

> The most recent intermittent failure showed that the error occurred during VM 
> initialization.
> Only the tty output was diverted, but not log output.
> Add diversion of log output as well tty output.
> 
> Add `-Xlog:all=warning:stderr` and `-Xlog:all=warning:stdout` to correspond 
> to 
> `-XX:+DisplayVMOutputToStderr` and `-XX:+DisplayVMOutputToStdout`

This pull request has now been integrated.

Changeset: 7b61a426
Author:Roger Riggs 
URL:   https://git.openjdk.java.net/jdk/commit/7b61a426
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod

8265173: [test] divert spurious log output away from stream under test in 
ProcessBuilder Basic test

Reviewed-by: dholmes

-

PR: https://git.openjdk.java.net/jdk/pull/3492

Re: RFR: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test

[Roger Riggs]

On Wed, 14 Apr 2021 14:08:50 GMT, Roger Riggs  wrote:

> The most recent intermittent failure showed that the error occurred during VM 
> initialization.
> Only the tty output was diverted, but not log output.
> Add diversion of log output as well tty output.
> 
> Add `-Xlog:all=warning:stderr` and `-Xlog:all=warning:stdout` to correspond 
> to 
> `-XX:+DisplayVMOutputToStderr` and `-XX:+DisplayVMOutputToStdout`

I checked the other uses of -XX:+Display... and they work as intended.
Thanks

-

PR: https://git.openjdk.java.net/jdk/pull/3492

Re: RFR: 8264208: Console charset API [v8]

[Alan Bateman]

On Wed, 14 Apr 2021 17:17:03 GMT, Naoto Sato  wrote:

>> Please review the changes for the subject issue.  This has been suggested in 
>> a recent discussion thread for the JEP 400 
>> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>>  A CSR has also been drafted, and comments are welcome 
>> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added @see links.

src/java.base/share/classes/java/io/Console.java line 397:

> 395: /**
> 396:  * Returns the {@link java.nio.charset.Charset Charset} object used 
> in
> 397:  * the {@code Console}.

What would you think about re-phrasing the first sentence to use "for the 
Console" rather than "in the Console".

src/java.base/share/classes/java/lang/System.java line 123:

> 121:  *
> 122:  * @see Console#charset()
> 123:  * @see Console#reader()

What would you think about changing the example in InputStreamReader class 
description as part of this?

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Integrated: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

[Conor Cleary]

On Fri, 9 Apr 2021 13:15:16 GMT, Conor Cleary  wrote:

> ### Description
> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
> the details of which can be seen in 
> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
> only a single object unique to the requirements of the method is used. The 
> issues these occurrences of AICs cause are highlighted below.
> 
> - AICs, in the cases concerned with this fix, are used where only one 
> operation is required. While AICs can be useful for more complex 
> implementations (using interfaces, multiple methods needed, local fields 
> etc.), Lambdas are better suited here as they result in a more readable and 
> concise solution.
> 
> ### Fixes
> - Where applicable, occurrences of AICs were replaced with lambdas to address 
> the issues above resulting primarily in more readable/concise code.

This pull request has now been integrated.

Changeset: 4e90d740
Author:Conor Cleary 
Committer: Aleksei Efimov 
URL:   https://git.openjdk.java.net/jdk/commit/4e90d740
Stats: 84 lines in 5 files changed: 0 ins; 48 del; 36 mod

8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI

Reviewed-by: rriggs, dfuchs, aefimov, chegar

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: Enhancement proposal regarding bufferization of InputStream

[Roger Riggs]

Hi Sergey,

Are you taking into account that for many reads the data is not copied 
into the local buffer.

See the comments in BufferedInputStream.read1: 277:280?

How much is the slowdown, when BufferedInputStreams are chained?

Thanks, Roger

On 4/15/21 7:08 AM, Pavel Rappo wrote:

On 15 Apr 2021, at 08:33, Сергей Цыпанов  wrote:

Hello,

buffering with j.i.BufferedInputStream is a handy way to improve performance of 
IO operations.
However in many cases buffering is redundant. Consider this code snippet from 
Spring Framework:

static ClassReader getClassReader(Resource rsc) throws Exception {
try (var is = new BufferedInputStream(rsc.getInputStream())) {
   return new ClassReader(is);
}
}

Interface Resource has lots of implementations some of them return buffered 
InputStream,
others don't, but all of them get wrapped with BufferedInputStream.

Apart from obvious misuses like BufferedInputStream cascade such wrapping is 
not necessary,
e.g. when we read a huge file using FileInputStream.readAllBytes(),
in others cases it's harmful e.g. when we read a small (comparing to the default
size of buffer in BufferedInputStream) file with readAllBytes() or
when we read from ByteArrayInputStream which is kind of buffered one by its 
nature.

I think an instance of InputStream should decide itself whether it requires 
buffering,
so I suggest to add a couple of methods into j.i.InputStream:

// subclasses can override this
protected boolean needsBuffer() {
return true;
}

public InputStream buffered() {
return needsBuffer() ? new BufferedInputStream(this) : this;
}

this allows subclasses of InputStream to override needsBuffer() to declare 
buffering redundancy.
Let's imagine we've overridden needsBuffer() in BufferedInputStream:

public class BufferedInputStream {
@Override
public boolean needsBuffer() {
   return true;
}
}

then the code we've mentioned above should be rewritten as

static ClassReader getClassReader(Resource rsc) throws Exception {
try (var is = rsc.getInputStream().buffered()) {
   return new ClassReader(is);
}
}

preventing cascade of BufferedInputStreams.


When I read this part


There are also cases when the need for buffering depends on the way how we read 
from InputStream:

new FileInputStream(file).buffered().readAllBytes() // buffering is redundant

my knee-jerk reaction was that a better solution likely lies with introducing a 
marker interface and selectively implementing it as opposed to adding two new 
methods to the existing class and selectively overriding them. Let's call this 
interface java.io.Buffered: Bufferred is to InputStream as RandomAccess is to 
List.

Just to be clear: I'm not proposing to actually do this. It's just a thought.

-Pavel


var data = new DataInputStream(new FileInputStream(file).buffered())
for (int i = 0; i < 1000; i++) {
  data.readInt();  // readInt() does 4 calls to 
InputStream.read() so buffering is needed
}

here if FileInputStream.needsBuffer() is overridden and returns false (assuming 
most of reads from it are bulk)
then we won't have buffering for DataInputStream. To avoid this we can also
add InputStream.buffered(boolean enforceBuffering) to have manual control.

To sum up, my proposal is to add those methods to InputStream:

protected static boolean needsBuffer() {
return true;
}

public InputStream buffered() {
return buffered(needsBuffer());
}

public InputStream buffered(boolean enforceBuffering) {
return enforceBuffering ? new BufferedInputStream(this) : this;
}

What do you think?

With best regards,
Sergey Tsypanov

RFR: 8265279: Remove unused RandomGeneratorFactory.all(Class category)

[Jim Laskey]

No longer needed

-

Commit messages:
 - RandomGeneratorFactory.all(Class category) no longer needed
 - Remove extraneous references to makeXXXSpliterator
 - Move makeXXXSpliterator methods to RandomSupport
 - change static final from 'proxy' to 'PROXY'
 - Make makeXXXSpliterator final
 - Move makeXXXSpilterator from public (@hidden) to protected. No API change.

Changes: https://git.openjdk.java.net/jdk/pull/3516/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3516=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265279
  Stats: 229 lines in 5 files changed: 19 ins; 184 del; 26 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3516.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3516/head:pull/3516

PR: https://git.openjdk.java.net/jdk/pull/3516

Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v6]

[Lin Zang]

> 4890732: GZIPOutputStream doesn't support optional GZIP fields

Lin Zang 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 ten additional commits since the 
last revision:

 - Merge remote-tracking branch 'upstream/master' into gzip-field
 - remove trailing spaces
 - Use record and Builder pattern
 - add class GZIPHeaderData, refine testcases
 - update copyright
 - reuse arguments constructor for non-argument one.
 - add test case
 - remove trailing spaces
 - 4890732: support optional GZIP fields in GZIPOutputStream

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3072/files
  - new: https://git.openjdk.java.net/jdk/pull/3072/files/35eb55eb..24c1b45f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3072=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3072=04-05

  Stats: 78610 lines in 2885 files changed: 49815 ins; 18293 del; 10502 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3072.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3072/head:pull/3072

PR: https://git.openjdk.java.net/jdk/pull/3072

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v5]

[Jim Laskey]

> Move makeXXXSpilterator from public (@hidden) to protected. No API ch

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove extraneous references to makeXXXSpliterator

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3469/files
  - new: https://git.openjdk.java.net/jdk/pull/3469/files/0094c43a..d72575d5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3469=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3469=03-04

  Stats: 32 lines in 1 file changed: 0 ins; 31 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3469/head:pull/3469

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v3]

[Lin Zang]

On Wed, 24 Mar 2021 10:25:44 GMT, Lance Andersen  wrote:

>> Lin Zang has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - update copyright
>>  - reuse arguments constructor for non-argument one.
>
> Hi Lin,
> 
> On Mar 24, 2021, at 2:51 AM, Lin Zang ***@***.**@***.***>> wrote:
> 
> 
> 
> Hi Lance，
> Thanks a lot for your review. I will update the PR ASAP.
> May I ask your help to also review the CSR？
> 
> I believe we need to flush out some of the issues I raised that were not test 
> related as they will result in updates to the javadoc which will require an 
> update to the CSR.
> 
> 
> 
> Thanks!
> 
> BRs,
> Lin
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on 
> GitHub, or 
> unsubscribe.
> 
> ***@***.***
> 
> 
> 
> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> ***@***.**@***.***>

Dear @LanceAndersen, 
Thanks a lot for your guidance, I have updated a commit that use record and 
builder pattern. 
In summary it creates a class named `GZIPHeaderBuilder`, and a **recor**d named 
`GZIPHeaderData`. 

The `GZIPHeaderData` **record** holds the header `flags`, `extra field`, `file 
name`, `file comments`, `header crc` and a byte array that contains the 
generated header data. 

The `GZIPHeaderBuilder` is mainly used for generating the `GZIPHeaderData`.  

User could set `optional gzip header fields` by calling different api like 
`withFileName()` of `GZIPHeaderBuilder` and generate the `GZIPHeaderData` 
finally by calling build().

Would you like to help review whether this update looks better?

Thanks,
Lin

-

PR: https://git.openjdk.java.net/jdk/pull/3072

Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v5]

[Lin Zang]

> 4890732: GZIPOutputStream doesn't support optional GZIP fields

Lin Zang has updated the pull request incrementally with one additional commit 
since the last revision:

  Use record and Builder pattern

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3072/files
  - new: https://git.openjdk.java.net/jdk/pull/3072/files/03b3e966..35eb55eb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3072=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3072=03-04

  Stats: 692 lines in 4 files changed: 363 ins; 323 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3072.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3072/head:pull/3072

PR: https://git.openjdk.java.net/jdk/pull/3072

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v4]

[Jim Laskey]

On Thu, 15 Apr 2021 12:11:56 GMT, Uwe Schindler  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Move makeXXXSpliterator methods to RandomSupport
>
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 
> 1656:
> 
>> 1654: // Methods required by class AbstractSpliteratorGenerator
>> 1655: 
>> 1656: protected Spliterator.OfInt makeIntsSpliterator(long index, 
>> long fence, int origin, int bound) {
> 
> Are those still needed? It looks like this was not detected because of the 
> missing `@Override`. Those could also be private or removed at all? The code 
> is also handled by the above private versions (`this` is not instanceof 
> `ThreadLocalRandom`).

True that. There are also some mentions in the comments.

-

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v4]

[Uwe Schindler]

On Thu, 15 Apr 2021 12:01:07 GMT, Jim Laskey  wrote:

>> Move makeXXXSpilterator from public (@hidden) to protected. No API ch
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move makeXXXSpliterator methods to RandomSupport

src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 
1656:

> 1654: // Methods required by class AbstractSpliteratorGenerator
> 1655: 
> 1656: protected Spliterator.OfInt makeIntsSpliterator(long index, 
> long fence, int origin, int bound) {

Are those still needed? It looks like because of the missing `@Override` those 
could also be private?

-

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v4]

[Jim Laskey]

> Move makeXXXSpilterator from public (@hidden) to protected. No API ch

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Move makeXXXSpliterator methods to RandomSupport

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3469/files
  - new: https://git.openjdk.java.net/jdk/pull/3469/files/c6b5da30..0094c43a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3469=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3469=02-03

  Stats: 87 lines in 2 files changed: 40 ins; 44 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3469/head:pull/3469

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Florent Guillaume]

On Wed, 14 Apr 2021 22:23:57 GMT, Peter Levart  wrote:

>> src/java.base/share/classes/java/lang/String.java line 3230:
>> 
>>> 3228: 
>>> 3229: /**
>>> 3230:  * Designated join routine.
>> 
>> Did you mean "dedicated"?
>
> No, I meant designated. It is the routine that all other public API entry 
> points call at the end to do the job. Would some other word more accurately 
> describe that? I definitely didn't mean "dedicated".

Oh then sorry, I thought it was a typo of some sort. I'd have said something 
like "Centralized join logic". But whatever works for you.

-

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: Enhancement proposal regarding bufferization of InputStream

[Pavel Rappo]

> On 15 Apr 2021, at 08:33, Сергей Цыпанов  wrote:
> 
> Hello,
> 
> buffering with j.i.BufferedInputStream is a handy way to improve performance 
> of IO operations.
> However in many cases buffering is redundant. Consider this code snippet from 
> Spring Framework:
> 
> static ClassReader getClassReader(Resource rsc) throws Exception {
> try (var is = new BufferedInputStream(rsc.getInputStream())) {
>   return new ClassReader(is);
> }
> }
> 
> Interface Resource has lots of implementations some of them return buffered 
> InputStream,
> others don't, but all of them get wrapped with BufferedInputStream.
> 
> Apart from obvious misuses like BufferedInputStream cascade such wrapping is 
> not necessary,
> e.g. when we read a huge file using FileInputStream.readAllBytes(),
> in others cases it's harmful e.g. when we read a small (comparing to the 
> default 
> size of buffer in BufferedInputStream) file with readAllBytes() or
> when we read from ByteArrayInputStream which is kind of buffered one by its 
> nature.
> 
> I think an instance of InputStream should decide itself whether it requires 
> buffering,
> so I suggest to add a couple of methods into j.i.InputStream:
> 
> // subclasses can override this
> protected boolean needsBuffer() {
> return true;
> }
> 
> public InputStream buffered() {
> return needsBuffer() ? new BufferedInputStream(this) : this;
> }
> 
> this allows subclasses of InputStream to override needsBuffer() to declare 
> buffering redundancy.
> Let's imagine we've overridden needsBuffer() in BufferedInputStream:
> 
> public class BufferedInputStream {
> @Override
> public boolean needsBuffer() {
>   return true;
> }
> }
> 
> then the code we've mentioned above should be rewritten as
> 
> static ClassReader getClassReader(Resource rsc) throws Exception {
> try (var is = rsc.getInputStream().buffered()) {
>   return new ClassReader(is);
> }
> }
> 
> preventing cascade of BufferedInputStreams.
> 

When I read this part

> There are also cases when the need for buffering depends on the way how we 
> read from InputStream:
> 
> new FileInputStream(file).buffered().readAllBytes() // buffering is redundant

my knee-jerk reaction was that a better solution likely lies with introducing a 
marker interface and selectively implementing it as opposed to adding two new 
methods to the existing class and selectively overriding them. Let's call this 
interface java.io.Buffered: Bufferred is to InputStream as RandomAccess is to 
List.

Just to be clear: I'm not proposing to actually do this. It's just a thought.

-Pavel

> var data = new DataInputStream(new FileInputStream(file).buffered())
> for (int i = 0; i < 1000; i++) {
>  data.readInt();  // readInt() does 4 calls to 
> InputStream.read() so buffering is needed
> }
> 
> here if FileInputStream.needsBuffer() is overridden and returns false 
> (assuming most of reads from it are bulk)
> then we won't have buffering for DataInputStream. To avoid this we can also
> add InputStream.buffered(boolean enforceBuffering) to have manual control.
> 
> To sum up, my proposal is to add those methods to InputStream:
> 
> protected static boolean needsBuffer() {
> return true;
> }
> 
> public InputStream buffered() {
> return buffered(needsBuffer());
> }
> 
> public InputStream buffered(boolean enforceBuffering) {
> return enforceBuffering ? new BufferedInputStream(this) : this;
> }
> 
> What do you think?
> 
> With best regards,
> Sergey Tsypanov

Enhancement proposal regarding bufferization of InputStream

[Сергей Цыпанов]

Hello,

buffering with j.i.BufferedInputStream is a handy way to improve performance of 
IO operations.
However in many cases buffering is redundant. Consider this code snippet from 
Spring Framework:

static ClassReader getClassReader(Resource rsc) throws Exception {
 try (var is = new BufferedInputStream(rsc.getInputStream())) {
   return new ClassReader(is);
 }
}

Interface Resource has lots of implementations some of them return buffered 
InputStream,
others don't, but all of them get wrapped with BufferedInputStream.

Apart from obvious misuses like BufferedInputStream cascade such wrapping is 
not necessary,
e.g. when we read a huge file using FileInputStream.readAllBytes(),
in others cases it's harmful e.g. when we read a small (comparing to the 
default 
size of buffer in BufferedInputStream) file with readAllBytes() or
when we read from ByteArrayInputStream which is kind of buffered one by its 
nature.

I think an instance of InputStream should decide itself whether it requires 
buffering,
so I suggest to add a couple of methods into j.i.InputStream:

// subclasses can override this
protected boolean needsBuffer() {
 return true;
}

public InputStream buffered() {
 return needsBuffer() ? new BufferedInputStream(this) : this;
}

this allows subclasses of InputStream to override needsBuffer() to declare 
buffering redundancy.
Let's imagine we've overridden needsBuffer() in BufferedInputStream:

public class BufferedInputStream {
 @Override
 public boolean needsBuffer() {
   return true;
 }
}

then the code we've mentioned above should be rewritten as

static ClassReader getClassReader(Resource rsc) throws Exception {
 try (var is = rsc.getInputStream().buffered()) {
   return new ClassReader(is);
 }
}

preventing cascade of BufferedInputStreams.

There are also cases when the need for buffering depends on the way how we read 
from InputStream:

new FileInputStream(file).buffered().readAllBytes() // buffering is redundant

var data = new DataInputStream(new FileInputStream(file).buffered())
for (int i = 0; i < 1000; i++) {
  data.readInt();  // readInt() does 4 calls to 
InputStream.read() so buffering is needed
}

here if FileInputStream.needsBuffer() is overridden and returns false (assuming 
most of reads from it are bulk)
then we won't have buffering for DataInputStream. To avoid this we can also
add InputStream.buffered(boolean enforceBuffering) to have manual control.

To sum up, my proposal is to add those methods to InputStream:

protected static boolean needsBuffer() {
 return true;
}

public InputStream buffered() {
 return buffered(needsBuffer());
}

public InputStream buffered(boolean enforceBuffering) {
 return enforceBuffering ? new BufferedInputStream(this) : this;
}

What do you think?

With best regards,
Sergey Tsypanov