Re: MatchResult support for named groups

[Cay Horstmann]

Hi Stuart,

1: If there is no match at all, then results yields the empty stream. I 
don't think anything else is required.


2/3: I wrote a fair number of regex patterns in Java, ever since they 
appeared in 2002. I can say with confidence that I never once used 
hitEnd/requireEnd, or seen it used. I note they occur in one file in the 
JDK 11 source, in the Scanner class. But not in a loop that fetches all 
matches, but after a single call to find or lookingAt. I think these are 
exceedingly uncommon.


In contrast, looping over matcher.find() and extracting groups is 
common, and named groups are a best practice 
(https://eslint.org/docs/rules/prefer-named-capture-group).


Cheers,

Cay

Il 04/12/2020 19:53, Stuart Marks ha scritto:

Hi Cay,

Thanks for mentioning this. It's good to know that adding this provides 
value to people who are actually trying to use this stuff (as opposed to 
adding stuff merely for the sake of completeness, as often seems to arise).


I've added some notes to JDK-8065554.

Looking at this more closely, it seems to me that MatchResult ought to 
include more match-result-related information that's currently only in 
Matcher, namely:


1. whether there was a match at all
2. hitEnd
3. requireEnd

If you have any thoughts on these, please let me know.

s'marks

On 12/2/20 2:53 AM, Cay Horstmann wrote:

Hello, I'd like to raise awareness for

https://bugs.openjdk.java.net/browse/JDK-8180352
https://bugs.openjdk.java.net/browse/JDK-8072984
https://bugs.openjdk.java.net/browse/JDK-8065554

These all ask for MatchResult.group(String name). What they don't 
mention is that this is more urgent in light of the methods


Stream Matcher.results() // 
https://bugs.openjdk.java.net/browse/JDK-8071479
Stream Scanner.findAll(Pattern pattern) // 
https://bugs.openjdk.java.net/browse/JDK-8072722


In particular, Matcher.results() seems a cleaner way of collecting 
match results than calling while (matcher.find()).


But then MatchResult needs to support the same queries that Matcher 
provides. I believe the only missing one is group(String name).


Cheers,

Cay

NB. There are related requests that ask for finding group names in 
patterns, or for correlating group names and numbers. I have formed no 
opinion on their merits.




--

Cay S. Horstmann | http://horstmann.com | mailto:c...@horstmann.com

RFR: 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource

[Alexander Matveev]

Fixed by using correct localized resources.

-

Commit messages:
 - 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl 
resource

Changes: https://git.openjdk.java.net/jdk/pull/1638/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1638=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255619
  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1638.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1638/head:pull/1638

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

Re: RFR: 8257450: Start of release updates for JDK 17

[Mikael Vidstedt]

On Tue, 1 Dec 2020 06:22:51 GMT, Joe Darcy  wrote:

> Start of JDK 17 updates.

Marked as reviewed by mikael (Reviewer).

-

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

Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal

[Dan Smith]

On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith  wrote:

> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100).
> 
> Development has been broken into 5 tasks, each with its own JBS issue:
> - Deprecate wrapper class constructors for removal
> - Revise "value-based class" & apply to wrappers
> - Define & apply annotation jdk.internal.ValueBased
> - Add 'lint' warning for @ValueBased classes
> - Diagnose synchronization on @ValueBased classes
> 
> All changes have been previously reviewed and integrated into the [`jep390` 
> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` 
> repository. See the subtasks of the 5 JBS issues for these changes, including 
> discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, 
> rriggs, lfoltan, fparain, hseigel.)
> 
> CSRs have also been completed or are nearly complete:
> 
> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper 
> class constructor deprecation
> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for 
> revisions to "value-based class"
> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new 
> `javac` lint warnings
> 
> Here's an overview of the files changed:
> 
> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to 
> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances 
> previous work that produced such diagnostics for the primitive wrapper 
> classes.
> 
> - `src/java.base/share/classes/java/lang`: deprecating for removal the 
> wrapper class constructors; revising the definition of "value-based class" in 
> `ValueBased.html` and the description used by linking classes; applying 
> "value-based class" to the primitive wrapper classes; marking value-based 
> classes with `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/lang/constant`: no longer designating 
> these classes as "value-based", since they rely heavily on field inheritance.
> 
> - `src/java.base/share/classes/java/time`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/util`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/jdk/internal`: define the 
> `@jdk.internal.ValueBased` annotation.
> 
> - `src/java.management.rmi`: removing uses of wrapper class constructors.
> 
> - `src/java.xml`: removing uses of wrapper class constructors.
> 
> - `src/jdk.compiler`: implementing the `synchronization` lint category, which 
> reports attempts to synchronize on classes and interfaces annotated with 
> `@jdk.internal.ValueBased`.
> 
> - `src/jdk.incubator.foreign`: revising the description used to link to 
> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a 
> special module export, which was not deemed necessary for an incubating API.)
> 
> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and 
> synchronization warnings in tests
> 
> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics
> 
> - `test`: testing new `javac` and HotSpot behavior; removing usages of 
> deprecated wrapper class constructors from tests, or suppressing deprecation 
> warnings; revising the description used to link to `ValueBased.html`.

I've updated the description to provide an overview for those unfamiliar with 
this work. Reviewers are welcome, including those who have previously reviewed 
a subset of these changes.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 16:02:33 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> Nice clean description of the algorithm.

For what it's worth, the following code:

var list = Collections.nCopies(Integer.MAX_VALUE - 3, "");
var list2 = new ArrayList<>(list);
list2.addAll(list);

results in `java.lang.NegativeArraySizeException: -8` where the -8 is returned 
by `ArraysSupport.newLength`. This is a demonstration of the problem with 
overflow checking that is fixed by this change.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 19:13:45 GMT, Roger Riggs  wrote:

>> The preconditions aren't checked, because this is an internal method, and 
>> the code size is minimized in order to help inlining. That's also why 
>> `hugeLength` is a separate method. (I guess I could add comments to this 
>> effect.) With this in mind it's hard to reason about robustness. If 
>> prefLength is zero, this can only be because some precondition was violated 
>> (e.g., oldLength is negative). If this were to occur there doesn't seem to 
>> be any advantage choosing one undefined behavior over another.
>
> Is there a reason the code would not naturally work when either min or 
> preferred is zero?
> Why are their preconditions?  What do they allow?
> These methods are used in enough places that a slip in any of the clients 
> would be trouble and hard to track down.

The origin of this code is in collections like ArrayList that have an existing 
array (hence oldLength >= 0) and that need it to grow (hence minGrowth > 0). 
Those were the prevailing assumptions in the code from which this was derived, 
so they turn into preconditions here. I don't see the need to try to make this 
handle any more cases than it currently does. Doing so complicates the analysis 
and possibly the code as well. Certainly a bug in a caller might be difficult 
to track down, but I don't want to add argument checking or to provide 
"reasonable" behavior in the face of unreasonable inputs. This is an internal 
method; bugs in callers should be fixed.

-

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

Re: RFR: 8257516: define test group for manual tests [v3]

[Igor Ignatyev]

On Thu, 3 Dec 2020 22:54:13 GMT, Ivan Šipka  wrote:

>> Defined new test groups as defined in ticket. @fguallini
>
> Ivan Šipka has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8257516: define test group for manual tests

LGTM

-

Marked as reviewed by iignatyev (Reviewer).

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]

[Kim Barrett]

On Fri, 4 Dec 2020 22:38:24 GMT, Mandy Chung  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> Mandy Chung has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - revise matchsKey to check equality if the reference is not cleared
>  - fix typo
>  - update copyright end year and fixup grammatical nit
>  - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
>  - 8256167: Convert JDK use of  to
>  - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
>  - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
>  - minor cleanup
>  - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into 
> refersTo
>  - improve refersTo0 descriptions
>  - ... and 5 more: 
> https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3

Marked as reviewed by kbarrett (Reviewer).

-

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

RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal

[Dan Smith]

Integration of JEP 390, addressing the following issues:

8252180: [JEP 390] Deprecate wrapper class constructors for removal
8254047: [JEP 390] Revise "value-based class" & apply to wrappers
8252181: [JEP 390] Define & apply annotation jdk.internal.ValueBased
8252183: [JEP 390] Add 'lint' warning for @ValueBased classes
8257027: [JEP 390] Diagnose synchronization on @ValueBased classes

See https://github.com/openjdk/valhalla/tree/jep390 for development history.

-

Commit messages:
 - Merge
 - 8257776: [valhalla:jep390] Add disclaimer about future changes to 
value-based classes
 - 8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers
 - 8252182: [JEP 390] Diagnose synchronization on @ValueBased classes
 - Merge
 - 8256663: [test] Deprecated use of new Double in jshell ImportTest
 - 8253962: Add @ValueBased to unmodifable Collection implementation classes
 - 8256002: Cleanup of Wrapper changes
 - Merge
 - 8254271: Development to deprecate wrapper class constructors for removal
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/93b6ab56...7c5e5bfe

Changes: https://git.openjdk.java.net/jdk/pull/1636/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1636=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252180
  Stats: 902 lines in 114 files changed: 489 ins; 121 del; 292 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1636.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1636/head:pull/1636

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

Re: RFR: 8257450: Start of release updates for JDK 17

[Iris Clark]

On Tue, 1 Dec 2020 06:22:51 GMT, Joe Darcy  wrote:

> Start of JDK 17 updates.

Marked as reviewed by iris (Reviewer).

-

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

Re: RFR: 8243614: Typo in ReentrantLock's Javadoc

[David Holmes]

On Fri, 4 Dec 2020 22:06:03 GMT, Martin Buchholz  wrote:

> Add missing semicolon
> 
> Martin's first github pr.

Looks trivially fine.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]

[Mandy Chung]

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Mandy Chung has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 15 commits:

 - revise matchsKey to check equality if the reference is not cleared
 - fix typo
 - update copyright end year and fixup grammatical nit
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - 8256167: Convert JDK use of  to
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - minor cleanup
 - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into 
refersTo
 - improve refersTo0 descriptions
 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3

-

Changes: https://git.openjdk.java.net/jdk/pull/1609/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1609=01
  Stats: 60 lines in 12 files changed: 7 ins; 11 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1609.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609

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

RFR: 8243614: Typo in ReentrantLock's Javadoc

[Martin Buchholz]

Add missing semicolon

Martin's first github pr.

-

Commit messages:
 - JDK-8243614-typo

Changes: https://git.openjdk.java.net/jdk/pull/1633/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1633=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8243614
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1633.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1633/head:pull/1633

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

Re: RFR: 8251989: Hex formatting and parsing utility [v17]

[Roger Riggs]

On Fri, 4 Dec 2020 20:39:58 GMT, Joe Darcy  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add class level clarification of use of uppercase for primitive conversions
>>   Switched order of declaration of a couple of method to make the javadoc 
>> sequence easier to read
>
> src/java.base/share/classes/java/util/HexFormat.java line 936:
> 
>> 934:  *  if any of the characters is not a hexadecimal character
>> 935:  */
>> 936: public int fromHexDigits(CharSequence string) {
> 
> I recommend this group of methods include an apinote explaining the 
> differences in behavior of compared to parseInt(s, 16) and 
> parseUnsignedInt(s, 16).

Will add:
 * @apiNote
 * {@link Integer#parseInt(String, int) Integer.parseInt(s, 16)} and
 * {@link Integer#parseUnsignedInt(String, int) Integer.pareUnsignedInt(s, 
16)}
 * are similar but allow all Unicode hexadecimal digits allowed by
 * {@link Character#digit(char, int) Character.digit(ch, 16)}.
 * {@code HexFormat} uses only Latin1 hexadecimal characters "0-9, "A-F", 
and "a-f".
 * {@link Integer#parseInt(String, int)} can parse signed hexadecimal 
strings.
And similar text for Long#parseLong and Long.parseUnsignedLong

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 20:14:13 GMT, Peter Levart  wrote:

>> Good point on checking k != null. A cleaner check:
>> 
>> // then check for equality if the referent is not cleared
>> Object k = e.get();
>> return k != null || key.equals(k);
>
> You meant: `return k != null && key.equals(k);` right?

oops...yes.

-

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

Re: RFR: 8257450: Start of release updates for JDK 17

[Erik Joelsson]

On Tue, 1 Dec 2020 06:22:51 GMT, Joe Darcy  wrote:

> Start of JDK 17 updates.

Build changes look good and I'm happy there are so few of them!

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8257450: Start of release updates for JDK 17

[Joe Darcy]

On Tue, 1 Dec 2020 07:38:13 GMT, David Holmes  wrote:

>> Start of JDK 17 updates.
>
> Looks good - 99% sym stuff :)
> 
> Doesn't look like the hotspot deprecated flag test will need updating this 
> time, as no newly deprecated flags marked for obsoletion in 17.

Usual start of release update:

* Expected version number updates in the make system
* New set of data files to support --release 16 in javac
* Update to various version-related langtools APIs and tests
* Minor N -> N+1 updates in vm and vm tests

One javadoc test that failed under the new version was fixed before this PR was 
sent out.

-

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

Re: RFR: 8257450: Start of release updates for JDK 17

[Joe Darcy]

On Tue, 1 Dec 2020 07:33:25 GMT, David Holmes  wrote:

>> Start of JDK 17 updates.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 226:
> 
>> 224: REIFIABLE_TYPES_INSTANCEOF(JDK16, 
>> Fragments.FeatureReifiableTypesInstanceof, DiagKind.PLURAL),
>> 225: RECORDS(JDK16, Fragments.FeatureRecords, DiagKind.PLURAL),
>> 226: SEALED_CLASSES(JDK17, Fragments.FeatureSealedClasses, 
>> DiagKind.PLURAL),
> 
> Is this changed because it is still preview?

Right; JEP 397: "Sealed Classes (Second Preview)" is PTT for JDk 16.

-

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

Re: RFR: 8257450: Start of release updates for JDK 17

[David Holmes]

On Tue, 1 Dec 2020 06:22:51 GMT, Joe Darcy  wrote:

> Start of JDK 17 updates.

Looks good - 99% sym stuff :)

Doesn't look like the hotspot deprecated flag test will need updating this 
time, as no newly deprecated flags marked for obsoletion in 17.

src/hotspot/share/opto/type.cpp line 827:

> 825: tty->print("this meet t = ");  mt->dump(); tty->cr();
> 826: fatal("meet not commutative");
> 827:   }

Merge issue??

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 226:

> 224: REIFIABLE_TYPES_INSTANCEOF(JDK16, 
> Fragments.FeatureReifiableTypesInstanceof, DiagKind.PLURAL),
> 225: RECORDS(JDK16, Fragments.FeatureRecords, DiagKind.PLURAL),
> 226: SEALED_CLASSES(JDK17, Fragments.FeatureSealedClasses, 
> DiagKind.PLURAL),

Is this changed because it is still preview?

test/hotspot/gtest/aarch64/test_assembler_aarch64.cpp line 27:

> 25: #include "precompiled.hpp"
> 26: 
> 27: #if defined(AARCH64) && !defined(ZERO)

Another merge issue.

-

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

RFR: 8257450: Start of release updates for JDK 17

[Joe Darcy]

Start of JDK 17 updates.

-

Commit messages:
 - Update tests.
 - Merge branch 'master' into JDK-8257450
 - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into JDK-8257450
 - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into JDK-8257450
 - JDK-8257450
 - JDK-8257450
 - JDK-8257450

Changes: https://git.openjdk.java.net/jdk/pull/1531/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1531=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257450
  Stats: 7522 lines in 76 files changed: 7463 ins; 0 del; 59 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1531.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1531/head:pull/1531

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

Re: RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer

[Daniel Fuchs]

On Fri, 4 Dec 2020 19:44:33 GMT, Mahendra Chhipa 
 wrote:

> jaxp.library.SimpleHttpServer
> https://bugs.openjdk.java.net/browse/JDK-8212035

Changes requested by dfuchs (Reviewer).

test/jdk/sun/net/www/protocol/jar/MultiReleaseJarURLConnection.java line 80:

> 78: creator.buildSignedMultiReleaseJar();
> 79: 
> 80: server = new 
> SimpleHttpServer(TESTCONTEXT,System.getProperty("user.dir", "."));

Please add space after comma.

test/lib/jdk/test/lib/net/SimpleHttpServer.java line 95:

> 93: return _httpserver.getAddress().getPort();
> 94: }
> 95: 

There are many issues with this class - using "localhost" and binding to the 
wildcard address among others. 
Having instance variables that are not final but are accessed by potentially 
multiple threads is another. 
I could also mention not using try-with-resources or the odd _name convention.
It will need to be modernized if you want to put it in jdk.test.lib.net;

-

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

Re: RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer

[Daniel Fuchs]

On Fri, 4 Dec 2020 20:32:02 GMT, Daniel Fuchs  wrote:

>> jaxp.library.SimpleHttpServer
>> https://bugs.openjdk.java.net/browse/JDK-8212035
>
> test/jdk/sun/net/www/protocol/jar/MultiReleaseJarURLConnection.java line 80:
> 
>> 78: creator.buildSignedMultiReleaseJar();
>> 79: 
>> 80: server = new 
>> SimpleHttpServer(TESTCONTEXT,System.getProperty("user.dir", "."));
> 
> Please add space after comma.

I believe the InetAddress parameter should be preserved in order to allow test 
to specifically bind to the loopback address instead of the wildcard (binding 
to the wildcard has been a source of test instability in the past).

-

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

Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected

[Brent Christian]

On Fri, 4 Dec 2020 19:51:54 GMT, Mandy Chung  wrote:

>> Please review this fix for the intermittently-failing 
>> java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java.
>> 
>> The change replaces System.gc()+sleep() with the more robust gcAwait() 
>> mechanic used elsewhere in the test base, [as pointed out by 
>> Martin](https://bugs.openjdk.java.net/browse/JDK-8200102?focusedCommentId=14382648=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14382648)(thanks!).
>> 
>> The new version of the test passes 100 times out of 100 on the test farm.
>
> Alternatively, you can simply use `test/libjdk/test/lib/util/ForceGC` in the 
> testlibrary.

Yes, I think that's a better plan, Mandy.  Thanks.  I'll update the fix.

-

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

Re: RFR: 8251989: Hex formatting and parsing utility [v17]

[Joe Darcy]

On Wed, 2 Dec 2020 16:33:15 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat 
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> 
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add class level clarification of use of uppercase for primitive conversions
>   Switched order of declaration of a couple of method to make the javadoc 
> sequence easier to read

Marked as reviewed by darcy (Reviewer).

src/java.base/share/classes/java/util/HexFormat.java line 936:

> 934:  *  if any of the characters is not a hexadecimal character
> 935:  */
> 936: public int fromHexDigits(CharSequence string) {

I recommend this group of methods include an apinote explaining the differences 
in behavior of compared to parseInt(s, 16) and parseUnsignedInt(s, 16).

-

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

Re: RFR: 8251989: Hex formatting and parsing utility [v17]

[Joe Darcy]

On Wed, 2 Dec 2020 16:33:15 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat 
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> 
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add class level clarification of use of uppercase for primitive conversions
>   Switched order of declaration of a couple of method to make the javadoc 
> sequence easier to read

src/java.base/share/classes/java/util/HexFormat.java line 740:

> 738:  * @param value an {@code int} value
> 739:  * @return the eight hexadecimal characters for the {@code int} value
> 740:  */

I recommend added an @see link to Integer.toHexString (and Long.toHexString for 
the long overload).

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Peter Levart]

On Fri, 4 Dec 2020 20:16:14 GMT, Mandy Chung  wrote:

>> src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java
>>  line 127:
>> 
>>> 125: T got = get();
>>> 126: return got != null && wr.refersTo(got);
>>> 127: }
>> 
>> Here you could pre-screen the get() with:
>> 
>> if (this.refersTo(null) != wr.refersTo(null)) return false;
>> 
>> In case one of the two WeakReference(s) is cleared and the other is not, 
>> they are not equal and you don't call this.get() in such case.
>
> `expunge` is called at `get`, `put`, and `remove`.   While there is a chance 
> that a referent might be cleared,  I will leave this as is and this patch 
> simply replaces to use `refersTo`.

You're right. Where it would matter is in expunge() where the Map keys that get 
polled off the reference queue are all already cleared and when 
HashMap.remove(key) is called with such cleared key, it is this cleared key 
that is the target of key.equals(other) method invocation (and not vice versa), 
so it doesn't matter if .get() is called on such key as it is already cleared.

-

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

Integrated: 8257750: writeBuffer field of java.io.DataOutputStream should be final

[Brian Burkhalter]

On Fri, 4 Dec 2020 16:58:56 GMT, Brian Burkhalter  wrote:

> Please review this trivial change to make the `writeBuffer` array field final 
> and move it to the beginning of the class where other fields are declared.

This pull request has now been integrated.

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

8257750: writeBuffer field of java.io.DataOutputStream should be final

Reviewed-by: lancea, naoto

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 19:47:17 GMT, Peter Levart  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java
>  line 127:
> 
>> 125: T got = get();
>> 126: return got != null && wr.refersTo(got);
>> 127: }
> 
> Here you could pre-screen the get() with:
> 
> if (this.refersTo(null) != wr.refersTo(null)) return false;
> 
> In case one of the two WeakReference(s) is cleared and the other is not, they 
> are not equal and you don't call this.get() in such case.

`expunge` is called at `get`, `put`, and `remove`.   While there is a chance 
that a referent might be cleared,  I will leave this as is and this patch 
simply replaces to use `refersTo`.

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 20:05:23 GMT, Kevin Rushforth  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 437:
> 
>> 435: int index = indexFor(h, tab.length);
>> 436: Entry e = tab[index];
>> 437: while (e != null && !(e.hash == h && matchesKey(e, k))
> 
> This doesn't compile, which is why the checks from the GitHub actions build 
> failed.

I caught that and it's in my local repo that I haven't pushed the commit.  
Sorry for not syncing my branch for review.

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Peter Levart]

On Fri, 4 Dec 2020 20:09:01 GMT, Mandy Chung  wrote:

>> src/java.base/share/classes/java/util/WeakHashMap.java line 293:
>> 
>>> 291: // then checks for equality
>>> 292: Object k = e.get();
>>> 293: return key == k || key.equals(k);
>> 
>> I think `key == k` is already covered by refersTo. But k could be null; 
>> checking for that here might be useful, to skip the call to equals in that 
>> case.
>
> Good point on checking k != null. A cleaner check:
> 
> // then check for equality if the referent is not cleared
> Object k = e.get();
> return k != null || key.equals(k);

You meant: `return k != null && key.equals(k);` right?

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 18:12:36 GMT, Kim Barrett  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 293:
> 
>> 291: // then checks for equality
>> 292: Object k = e.get();
>> 293: return key == k || key.equals(k);
> 
> I think `key == k` is already covered by refersTo. But k could be null; 
> checking for that here might be useful, to skip the call to equals in that 
> case.

Good point on checking k != null. A cleaner check:

// then check for equality if the referent is not cleared
Object k = e.get();
return k != null || key.equals(k);

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Kevin Rushforth]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

You have a typo that will cause a compilation error.

src/java.base/share/classes/java/util/WeakHashMap.java line 437:

> 435: int index = indexFor(h, tab.length);
> 436: Entry e = tab[index];
> 437: while (e != null && !(e.hash == h && matchesKey(e, k))

This doesn't compile, which is why the checks from the GitHub actions build 
failed.

-

Changes requested by kcr (Author).

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

Re: RFR: 8255560: Class::isRecord should check that the current class is final and not abstract [v4]

[Joe Darcy]

On Wed, 2 Dec 2020 10:00:21 GMT, Chris Hegarty  wrote:

>> Update Class::isRecord to only return true for classes that are final.
>> 
>> The removal of non-specified JVM checks on classes with a Record Attribute 
>> (see JDK-8255342), has resulted in more types of loadable classes that may 
>> contain a Record Attribute. Since these checks are not performed by the JVM 
>> anymore, Class::isRecord, and by extension Class::getRecordComponents, may 
>> return true or component values, respectively, for classes that are not 
>> well-formed record classes (as per the JLS), .e.g. non-final or abstract 
>> classes, that contain a record Attribute.
>> 
>> Core Reflection, Class::isRecord, already asserts checks that the JVM does 
>> not, e.g. that the direct superclass is java.lang.Record. Some points from 
>> the Java Language Specification for record classes:
>> 
>>  1. It is a compile-time error if a record declaration has the modifier 
>> abstract.
>>  2. A record declaration is implicitly final.
>>  3. The direct superclass type of a record class is Record.
>> 
>> Class::isRecord already ensures no.3. This issue proposes to add explicit 
>> checks in Core Reflection to ensure no.1 and no.2, since the JVM now allows 
>> such classes that contain a Record Attribute to be loaded.
>
> Chris Hegarty has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix issue in ByteCodeLoader

Marked as reviewed by darcy (Reviewer).

-

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

Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected

[Mandy Chung]

On Fri, 4 Dec 2020 18:23:55 GMT, Brent Christian  wrote:

> Please review this fix for the intermittently-failing 
> java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java.
> 
> The change replaces System.gc()+sleep() with the more robust gcAwait() 
> mechanic used elsewhere in the test base, [as pointed out by 
> Martin](https://bugs.openjdk.java.net/browse/JDK-8200102?focusedCommentId=14382648=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14382648)(thanks!).
> 
> The new version of the test passes 100 times out of 100 on the test farm.

Alternatively, you can simply use `test/libjdk/test/lib/util/ForceGC` in the 
testlibrary.

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Peter Levart]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java
 line 127:

> 125: T got = get();
> 126: return got != null && wr.refersTo(got);
> 127: }

Here you could pre-screen the get() with:

if (this.refersTo(null) != wr.refersTo(null)) return false;

In case one of the two WeakReference(s) is cleared and the other is not, they 
are not equal and you don't call this.get() in such case.

-

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

RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer

[Mahendra Chhipa]

jaxp.library.SimpleHttpServer
https://bugs.openjdk.java.net/browse/JDK-8212035

-

Commit messages:
 - JDK-8212035 merge jdk.test.lib.util.SimpleHttpServer with

Changes: https://git.openjdk.java.net/jdk/pull/1632/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1632=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8212035
  Stats: 276 lines in 16 files changed: 72 ins; 141 del; 63 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1632.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1632/head:pull/1632

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Roger Riggs]

On Fri, 4 Dec 2020 17:49:25 GMT, Stuart Marks  wrote:

>> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 640:
>> 
>>> 638: int prefLength = oldLength + Math.max(minGrowth, prefGrowth); 
>>> // might overflow
>>> 639: if (0 < prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
>>> 640: return prefLength;
>> 
>> In spite of the assert `minGrowth > 0`, that is unchecked, I would suggest 
>> prefLength == 0 to return prefLength.
>> ```if (0 <= prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
>> return prefLength;```
>> Otherwise, it falls into hughLength(...) which will return the 
>> SOFT_MAX_ARRAY_LENGTH.
>> 
>> It would be more robust if the algorithm was well defined if either min or 
>> pref were zero.
>
> The preconditions aren't checked, because this is an internal method, and the 
> code size is minimized in order to help inlining. That's also why 
> `hugeLength` is a separate method. (I guess I could add comments to this 
> effect.) With this in mind it's hard to reason about robustness. If 
> prefLength is zero, this can only be because some precondition was violated 
> (e.g., oldLength is negative). If this were to occur there doesn't seem to be 
> any advantage choosing one undefined behavior over another.

Is there a reason the code would not naturally work when either min or 
preferred is zero?
Why are their preconditions?  What do they allow?
These methods are used in enough places that a slip in any of the clients would 
be trouble and hard to track down.

-

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

Re: MatchResult support for named groups

[Stuart Marks]

Hi Cay,

Thanks for mentioning this. It's good to know that adding this provides value to 
people who are actually trying to use this stuff (as opposed to adding stuff merely 
for the sake of completeness, as often seems to arise).


I've added some notes to JDK-8065554.

Looking at this more closely, it seems to me that MatchResult ought to include more 
match-result-related information that's currently only in Matcher, namely:


1. whether there was a match at all
2. hitEnd
3. requireEnd

If you have any thoughts on these, please let me know.

s'marks

On 12/2/20 2:53 AM, Cay Horstmann wrote:

Hello, I'd like to raise awareness for

https://bugs.openjdk.java.net/browse/JDK-8180352
https://bugs.openjdk.java.net/browse/JDK-8072984
https://bugs.openjdk.java.net/browse/JDK-8065554

These all ask for MatchResult.group(String name). What they don't mention is that 
this is more urgent in light of the methods


Stream Matcher.results() // 
https://bugs.openjdk.java.net/browse/JDK-8071479
Stream Scanner.findAll(Pattern pattern) // 
https://bugs.openjdk.java.net/browse/JDK-8072722


In particular, Matcher.results() seems a cleaner way of collecting match results 
than calling while (matcher.find()).


But then MatchResult needs to support the same queries that Matcher provides. I 
believe the only missing one is group(String name).


Cheers,

Cay

NB. There are related requests that ask for finding group names in patterns, or for 
correlating group names and numbers. I have formed no opinion on their merits.

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Martin Buchholz]

On Fri, 4 Dec 2020 16:54:26 GMT, Pavel Rappo  wrote:

>> @pavelrappo Please see my updated CSR below. Thanks.
>> 
>> # Map::compute should have the implementation requirement match its default 
>> implementation
>> 
>> ## Summary
>> 
>> The implementation requirement of Map::compute does not match its default 
>> implementation. Besides, it has some other minor issues. We should fix it.
>> 
>> ## Problem
>> 
>> The documentation of the implementation requirements for Map::compute has 
>> the following problems:
>> 1. It doesn't match its default implementation.
>> 1. It lacks of the return statements for most of the if-else cases.
>> 1. The indents are 3 spaces, while the convention is 4 spaces.
>> 1. The if-else is overly complicated and can be simplified.
>> 1. The surrounding prose contains incorrect statements.
>> 
>> ## Solution
>> 
>> Rewrite the documentation of Map::compute to match its default 
>> implementation and solve the above mentioned problems.
>> 
>> ## Specification
>> 
>> diff --git a/src/java.base/share/classes/java/util/Map.java 
>> b/src/java.base/share/classes/java/util/Map.java
>> index b1de34b42a5..b30e3979259 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1107,23 +1107,17 @@ public interface Map {
>>   *
>>   * @implSpec
>>   * The default implementation is equivalent to performing the following
>> - * steps for this {@code map}, then returning the current value or
>> - * {@code null} if absent:
>> + * steps for this {@code map}:
>>   *
>>   *  {@code
>>   * V oldValue = map.get(key);
>>   * V newValue = remappingFunction.apply(key, oldValue);
>> - * if (oldValue != null) {
>> - *if (newValue != null)
>> - *   map.put(key, newValue);
>> - *else
>> - *   map.remove(key);
>> - * } else {
>> - *if (newValue != null)
>> - *   map.put(key, newValue);
>> - *else
>> - *   return null;
>> + * if (newValue != null) {
>> + * map.put(key, newValue);
>> + * } else if (oldValue != null || map.containsKey(key)) {
>> + * map.remove(key);
>>   * }
>> + * return newValue;
>>   * }
>>   *
>>   * The default implementation makes no guarantees about detecting if 
>> the
>
> 1. @johnlinp I've created the CSR: 
> https://bugs.openjdk.java.net/browse/JDK-8257768
> 2. @dfuch, @stuart-marks, and 
> I-cannot-seem-to-find-Martin-Buchholz's-GitHub-username: care to review that 
> CSR?

Hello github - this is my first ever comment.

The javadoc specs for the various compute methods in all the Map classes should 
be maintained consistently and holistically.  Sorry for not having done so 
years ago.

Pavel is thinking about code snippets for all of OpenJDK today, while I have 
been thinking about code snippets in jsr166.  jsr166 code snippets have a 
consistent style that should also be used for Map.java, but we've had a mild 
case of maintainer style divergence.

-

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

RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected

[Brent Christian]

Please review this fix for the intermittently-failing 
java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java.

The change replaces System.gc()+sleep() with the more robust gcAwait() mechanic 
used elsewhere in the test base, [as pointed out by 
Martin](https://bugs.openjdk.java.net/browse/JDK-8200102?focusedCommentId=14382648=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14382648)(thanks!).

The new version of the test passes 100 times out of 100 on the test farm.

-

Commit messages:
 - Add gcAwait() mechanism

Changes: https://git.openjdk.java.net/jdk/pull/1630/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1630=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8200102
  Stats: 52 lines in 1 file changed: 41 ins; 5 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1630.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1630/head:pull/1630

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[stefan-zobel]

On Fri, 4 Dec 2020 06:50:14 GMT, Stuart Marks  wrote:

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 626:

> 624:  * attempt an array allocation with that length and encounter an 
> OutOfMemoryError.
> 625:  * Of course, regardless of the length value returned from this 
> method, the caller
> 626:  * may encounter OutOfMemoryError if there is insufficient heap to 
> fufill the request.

typo in line 626: fufill -> fulfill

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Kim Barrett]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

src/java.base/share/classes/java/util/WeakHashMap.java line 293:

> 291: // then checks for equality
> 292: Object k = e.get();
> 293: return key == k || key.equals(k);

I think `key == k` is already covered by refersTo. But k could be null; 
checking for that here might be useful, to skip the call to equals in that case.

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 09:19:23 GMT, Aleksey Shipilev  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 291:
> 
>> 289: if (e.refersTo(key)) return true;
>> 290: 
>> 291: // then checks for equality
> 
> Obnoxiously minor nit: plurality is inconsistent. `check if the given 
> entry...` above, and `then check[s] for equality`. Should be `...then check 
> for equality`?

OK.  Fixed this grammatical nit.

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v6]

[Mandy Chung]

On Fri, 4 Dec 2020 16:22:28 GMT, Jan Lahoda  wrote:

>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>> 
>> From the original PR:
>> 
>>> Please review the code for the second iteration of sealed classes. In this 
>>> iteration we are:
>>> 
>>> * Enhancing narrowing reference conversion to allow for stricter 
>>> checking of cast conversions with respect to sealed type hierarchies
>>> 
>>> * Also local classes are not considered when determining implicitly 
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>> 
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>>> still in the same method, the return type has been changed to Class[] 
>>> instead of the previous ClassDesc[]
>>> 
>>> * adding code to make sure that annotations can't be sealed
>>> 
>>> * improving some tests
>>> 
>>> 
>>> TIA
>>> 
>>> Related specs:
>>> [Sealed Classes 
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes 
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual 
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>> 
>> This PR strives to reflect the review comments from 1227:
>>  * adjustments to javadoc of j.l.Class methods
>>  * package access checks in Class.getPermittedSubclasses()
>>  * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Cleanup as suggested on the review.

Marked as reviewed by mchung (Reviewer).

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 15:53:01 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 640:
> 
>> 638: int prefLength = oldLength + Math.max(minGrowth, prefGrowth); 
>> // might overflow
>> 639: if (0 < prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
>> 640: return prefLength;
> 
> In spite of the assert `minGrowth > 0`, that is unchecked, I would suggest 
> prefLength == 0 to return prefLength.
> ```if (0 <= prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
> return prefLength;```
> Otherwise, it falls into hughLength(...) which will return the 
> SOFT_MAX_ARRAY_LENGTH.
> 
> It would be more robust if the algorithm was well defined if either min or 
> pref were zero.

The preconditions aren't checked, because this is an internal method, and the 
code size is minimized in order to help inlining. That's also why `hugeLength` 
is a separate method. (I guess I could add comments to this effect.) With this 
in mind it's hard to reason about robustness. If prefLength is zero, this can 
only be because some precondition was violated (e.g., oldLength is negative). 
If this were to occur there doesn't seem to be any advantage choosing one 
undefined behavior over another.

> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 70:
> 
>> 68: { IMAX-2, 1,  IMAX,   IMAX-1 },
>> 69: { IMAX-1, 1,  IMAX,   IMAX   }
>> 70: };
> 
> Adding test cases for zero (0) for the min and preferred would be good to 
> include and show any unpredictable behavior.

No, I don't want to add test cases that violate the preconditions. I guess I 
could add a prefGrowth==0 case though.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 15:47:51 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 654:
> 
>> 652: return SOFT_MAX_ARRAY_LENGTH;
>> 653: } else {
>> 654: return minLength;
> 
> Isn't this last `else if... then.. else` the same as:
> `return Math.max(minLength, SOFT_MAX_ARRAY_LENGTH)`

It is, and I considered replacing it, but I felt that it obscured what was 
going on.

-

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

Re: RFR: 8257750: writeBuffer field of java.io.DataOutputStream should be final [v2]

[Lance Andersen]

On Fri, 4 Dec 2020 17:22:29 GMT, Brian Burkhalter  wrote:

>> Please review this trivial change to make the `writeBuffer` array field 
>> final and move it to the beginning of the class where other fields are 
>> declared.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8257750: Change "byte writeBuffer[]" to "byte[] writeBuffer"

Marked as reviewed by lancea (Reviewer).

-

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

Re: RFR: 8257750: writeBuffer field of java.io.DataOutputStream should be final [v2]

[Brian Burkhalter]

> Please review this trivial change to make the `writeBuffer` array field final 
> and move it to the beginning of the class where other fields are declared.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8257750: Change "byte writeBuffer[]" to "byte[] writeBuffer"

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1627/files
  - new: https://git.openjdk.java.net/jdk/pull/1627/files/c994c07c..0ddd1068

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

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

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

Re: RFR: 8257750: writeBuffer field of java.io.DataOutputStream should be final

[Naoto Sato]

On Fri, 4 Dec 2020 16:58:56 GMT, Brian Burkhalter  wrote:

> Please review this trivial change to make the `writeBuffer` array field final 
> and move it to the beginning of the class where other fields are declared.

Marked as reviewed by naoto (Reviewer).

-

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

Re: RFR: 8257750: writeBuffer field of java.io.DataOutputStream should be final

[Lance Andersen]

On Fri, 4 Dec 2020 16:58:56 GMT, Brian Burkhalter  wrote:

> Please review this trivial change to make the `writeBuffer` array field final 
> and move it to the beginning of the class where other fields are declared.

Marked as reviewed by lancea (Reviewer).

-

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

RFR: 8257750: writeBuffer field of java.io.DataOutputStream should be final

[Brian Burkhalter]

Please review this trivial change to make the `writeBuffer` array field final 
and move it to the beginning of the class where other fields are declared.

-

Commit messages:
 - 8257750: writeBuffer field of java.io.DataOutputStream should be final

Changes: https://git.openjdk.java.net/jdk/pull/1627/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1627=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257750
  Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1627.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1627/head:pull/1627

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

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Pavel Rappo]

On Sat, 28 Nov 2020 08:42:52 GMT, John Lin 
 wrote:

>> @johnlinp In any case, you'd also need to update the surrounding prose; we 
>> need to remove this:
>>  returning the current value or {@code null} if absent:```
>
> @pavelrappo Please see my updated CSR below. Thanks.
> 
> # Map::compute should have the implementation requirement match its default 
> implementation
> 
> ## Summary
> 
> The implementation requirement of Map::compute does not match its default 
> implementation. Besides, it has some other minor issues. We should fix it.
> 
> ## Problem
> 
> The documentation of the implementation requirements for Map::compute has the 
> following problems:
> 1. It doesn't match its default implementation.
> 1. It lacks of the return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
> 1. The surrounding prose contains incorrect statements.
> 
> ## Solution
> 
> Rewrite the documentation of Map::compute to match its default implementation 
> and solve the above mentioned problems.
> 
> ## Specification
> 
> diff --git a/src/java.base/share/classes/java/util/Map.java 
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..b30e3979259 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1107,23 +1107,17 @@ public interface Map {
>   *
>   * @implSpec
>   * The default implementation is equivalent to performing the following
> - * steps for this {@code map}, then returning the current value or
> - * {@code null} if absent:
> + * steps for this {@code map}:
>   *
>   *  {@code
>   * V oldValue = map.get(key);
>   * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   map.remove(key);
> - * } else {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null || map.containsKey(key)) {
> + * map.remove(key);
>   * }
> + * return newValue;
>   * }
>   *
>   * The default implementation makes no guarantees about detecting if 
> the

1. @johnlinp I've created the CSR: 
https://bugs.openjdk.java.net/browse/JDK-8257768
2. @dfuch, @stuart-marks, and 
I-cannot-seem-to-find-Martin-Buchholz's-GitHub-username: care to review that 
CSR?

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v6]

[Chris Hegarty]

On Fri, 4 Dec 2020 16:22:28 GMT, Jan Lahoda  wrote:

>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>> 
>> From the original PR:
>> 
>>> Please review the code for the second iteration of sealed classes. In this 
>>> iteration we are:
>>> 
>>> * Enhancing narrowing reference conversion to allow for stricter 
>>> checking of cast conversions with respect to sealed type hierarchies
>>> 
>>> * Also local classes are not considered when determining implicitly 
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>> 
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>>> still in the same method, the return type has been changed to Class[] 
>>> instead of the previous ClassDesc[]
>>> 
>>> * adding code to make sure that annotations can't be sealed
>>> 
>>> * improving some tests
>>> 
>>> 
>>> TIA
>>> 
>>> Related specs:
>>> [Sealed Classes 
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes 
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual 
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>> 
>> This PR strives to reflect the review comments from 1227:
>>  * adjustments to javadoc of j.l.Class methods
>>  * package access checks in Class.getPermittedSubclasses()
>>  * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Cleanup as suggested on the review.

Marked as reviewed by chegar (Reviewer).

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v5]

[Jan Lahoda]

On Fri, 4 Dec 2020 13:25:04 GMT, Alan Bateman  wrote:

>> Jan Lahoda has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR.
>
> src/java.base/share/classes/java/lang/Class.java line 4394:
> 
>> 4392:  * implement this class or interface if it is sealed. The order of 
>> such elements
>> 4393:  * is unspecified. If this {@code Class} object represents a 
>> primitive type,
>> 4394:  * {@code void}, an array type, or a class or interface that is 
>> not sealed,
> 
> Did you mean {@code Void} here?

I think this means `void.class`. `void.class` is a little special (as are the 
Class objects for primitive types, like `int.class`), but Void.class or 
Integer.class are not so much special.

I've updated the patch based on the other comments - thanks!

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v6]

[Jan Lahoda]

> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
> 
> From the original PR:
> 
>> Please review the code for the second iteration of sealed classes. In this 
>> iteration we are:
>> 
>> * Enhancing narrowing reference conversion to allow for stricter 
>> checking of cast conversions with respect to sealed type hierarchies
>> 
>> * Also local classes are not considered when determining implicitly 
>> declared permitted direct subclasses of a sealed class or sealed interface
>> 
>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>> still in the same method, the return type has been changed to Class[] 
>> instead of the previous ClassDesc[]
>> 
>> * adding code to make sure that annotations can't be sealed
>> 
>> * improving some tests
>> 
>> 
>> TIA
>> 
>> Related specs:
>> [Sealed Classes 
>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>> [Sealed Classes 
>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>> [Additional: Contextual 
>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
> 
> This PR strives to reflect the review comments from 1227:
>  * adjustments to javadoc of j.l.Class methods
>  * package access checks in Class.getPermittedSubclasses()
>  * fixed to the narrowing conversion/castability as pointed out by Maurizio

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Cleanup as suggested on the review.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1483/files
  - new: https://git.openjdk.java.net/jdk/pull/1483/files/09e0c186..b5304057

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

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

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Roger Riggs]

On Fri, 4 Dec 2020 06:50:14 GMT, Stuart Marks  wrote:

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Nice clean description of the algorithm.

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 654:

> 652: return SOFT_MAX_ARRAY_LENGTH;
> 653: } else {
> 654: return minLength;

Isn't this last `else if... then.. else` the same as:
`return Math.max(minLength, SOFT_MAX_ARRAY_LENGTH)`

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 640:

> 638: int prefLength = oldLength + Math.max(minGrowth, prefGrowth); // 
> might overflow
> 639: if (0 < prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
> 640: return prefLength;

In spite of the assert `minGrowth > 0`, that is unchecked, I would suggest 
prefLength == 0 to return prefLength.
```if (0 <= prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
return prefLength;```
Otherwise, it falls into hughLength(...) which will return the 
SOFT_MAX_ARRAY_LENGTH.

It would be more robust if the algorithm was well defined if either min or pref 
were zero.

test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 70:

> 68: { IMAX-2, 1,  IMAX,   IMAX-1 },
> 69: { IMAX-1, 1,  IMAX,   IMAX   }
> 70: };

Adding test cases for zero (0) for the min and preferred would be good to 
include and show any unpredictable behavior.

-

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

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Pavel Rappo]

On Mon, 30 Nov 2020 15:08:51 GMT, Pavel Rappo  wrote:

>> @johnlinp, thanks for updating the CSR draft; it is much better now.
>> 
>> @stuart-marks, I think we could further improve this snippet. This `if` 
>> statement seems to use an optimization:
>> 
>> if (oldValue != null || map.containsKey(key))
>> 
>> I don't think we should include an optimization into the specification 
>> unless that optimization also improves readability. Is this the case here? 
>> Could this be better?
>> 
>> if (map.containsKey(key))
>
> I would even go as far as to rewrite that snippet like this:
> 
> if (newValue == null) {
> remove(key);
> } else {
> put(key, newValue);
> }
> return newValue;
> 
> This rewrite is possible thanks to the following properties of 
> `Map.remove(Object key)`:
> 
> 1. A call with an unmapped `key` has no effect.
> 2. A call with a mapped `key` has the same semantics regardless of the value 
> that this key is mapped to.
> 
> In particular, (2) covers `null` values.
> 
> To me, this rewrite reads better; however, I understand that readability is 
> subjective and that snippets used in `@implSpec` might be subject to 
> additional requirements.

> @pavelrappo The intended effect of the revised snippet is sensible, but again 
> I note that it differs from the actual default implementation. Specifically: 
> if the map does not contain the key and newValue is null, the default 
> implementation currently does nothing, whereas the revised snippet calls 
> `remove(key)`. This should have no effect _on the map_ but a subclass might 
> override `remove` and this behavior difference is observable. (The typical 
> example of this is maintaining a counter of the number of operations. I think 
> _Effective Java_ uses that example in discussing subclassing.) I think the 
> main priority here is fidelity to what the default implementation actually 
> does -- at least, concerning operations on _this_ -- and less on readability.

Although we should really have a conversation on code snippets in API 
specifications, this thread is not the place for that. However, I will 
minimally comment on some of what you've just said.

1. If a high-fidelity copy is not enough, an identical copy is required; that 
suggests a JavaDoc facility for embedding portions of code into doc comments.
2. I have to note that `Map.merge` (a method whose semantics is very close to 
that of `Map.compute`) is specified and implemented very similarly to what my 
[comment #1](https://github.com/openjdk/jdk/pull/714#issuecomment-735843488) 
proposed.

> The current snippet proposed by @johnlinp does seem to have the same behavior 
> as the default implementation; I would avoid trying to "optimize" this. 
> However, it does express the conditions and return value somewhat differently 
> from the way the default implementation does. I think those differences are 
> not significant to subclasses and are mostly stylistic. The original 
> `@implSpec` snippet attempted to handle the cases separately, whereas the 
> current proposed snippet minimizes them (while still agreeing with the 
> implementation's behavior). I'm not too concerned about this. I think the 
> current snippet is acceptable. Again, the main priority is agreement with the 
> implementation.

Perhaps there's some confusion. If anything my [comment 
#2](https://github.com/openjdk/jdk/pull/714#issuecomment-735798573) was 
proposing to _remove_ an optimization carried over from the default 
implementation.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 14:05:54 GMT, Erik Joelsson  wrote:

>> My understanding of JEPs is that they should be viewed as living documents. 
>> In this case, I think it's perfectly valid to update JEP 201 with additional 
>> source code layout. Both for this and for the other missing dirs.
>
> Regarding the chosen layout. Did you consider following the existing pattern 
> of src//{share,}/data?

@erikj79 Good point, that makes much more sense.

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v5]

[Maurizio Cimadamore]

On Fri, 4 Dec 2020 13:12:27 GMT, Jan Lahoda  wrote:

>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>> 
>> From the original PR:
>> 
>>> Please review the code for the second iteration of sealed classes. In this 
>>> iteration we are:
>>> 
>>> * Enhancing narrowing reference conversion to allow for stricter 
>>> checking of cast conversions with respect to sealed type hierarchies
>>> 
>>> * Also local classes are not considered when determining implicitly 
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>> 
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>>> still in the same method, the return type has been changed to Class[] 
>>> instead of the previous ClassDesc[]
>>> 
>>> * adding code to make sure that annotations can't be sealed
>>> 
>>> * improving some tests
>>> 
>>> 
>>> TIA
>>> 
>>> Related specs:
>>> [Sealed Classes 
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes 
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual 
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>> 
>> This PR strives to reflect the review comments from 1227:
>>  * adjustments to javadoc of j.l.Class methods
>>  * package access checks in Class.getPermittedSubclasses()
>>  * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

Compiler changes look good to me

-

Marked as reviewed by mcimadamore (Reviewer).

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Erik Joelsson]

On Fri, 4 Dec 2020 12:30:02 GMT, Alan Bateman  wrote:

>> And I can certainly move jdwp.spec to java.base instead. That's the reason I 
>> need input on this: All I know is that is definitely not the responsibility 
>> of the Build Group to maintain that document, and I made my best guess at 
>> where to place it.
>
>> And I can certainly move jdwp.spec to java.base instead. 
> 
> If jdwp.spec has to move to the src tree then src/java.se is probably the 
> most suitable home because Java SE specifies JDWP as an optional interface. 
> So nothing to do with java.base and the build will need to continue to 
> generate the sources for the front-end (jdk.jdi) and back-end 
> (jdk.jdwp.agent) as they implement the protocol.

My understanding of JEPs is that they should be viewed as living documents. In 
this case, I think it's perfectly valid to update JEP 201 with additional 
source code layout. Both for this and for the other missing dirs.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Erik Joelsson]

On Fri, 4 Dec 2020 14:03:08 GMT, Erik Joelsson  wrote:

>>> And I can certainly move jdwp.spec to java.base instead. 
>> 
>> If jdwp.spec has to move to the src tree then src/java.se is probably the 
>> most suitable home because Java SE specifies JDWP as an optional interface. 
>> So nothing to do with java.base and the build will need to continue to 
>> generate the sources for the front-end (jdk.jdi) and back-end 
>> (jdk.jdwp.agent) as they implement the protocol.
>
> My understanding of JEPs is that they should be viewed as living documents. 
> In this case, I think it's perfectly valid to update JEP 201 with additional 
> source code layout. Both for this and for the other missing dirs.

Regarding the chosen layout. Did you consider following the existing pattern of 
src//{share,}/data?

-

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

Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v2]

[Marius Volkhart]

> The default implementation of javax.xml.stream.XMLEventReader produced a 
> StartDocument event that always indicated that the "standalone" attribute was 
> set.
> 
> The root cause of this was that the 
> com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the 
> "standalone" attribute, rather than asking streamReader.standaloneSet() 
> before setting the property of the StartDocumentEvent being created.

Marius Volkhart has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains three commits:

 - Adjust test so it works with jtreg
 - Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT event
   
   The default implementation of javax.xml.stream.XMLEventReader produced a 
StartDocument event that always indicated that the "standalone" attribute was 
set.
   
   The root cause of this was that the 
com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the 
"standalone" attribute, rather than asking streamReader.standaloneSet() before 
setting the property of the StartDocumentEvent being created.
 - Add test for XmlInputFactory

-

Changes: https://git.openjdk.java.net/jdk/pull/1056/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1056=01
  Stats: 47 lines in 2 files changed: 46 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1056.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1056/head:pull/1056

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v5]

[Alan Bateman]

On Fri, 4 Dec 2020 13:12:27 GMT, Jan Lahoda  wrote:

>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>> 
>> From the original PR:
>> 
>>> Please review the code for the second iteration of sealed classes. In this 
>>> iteration we are:
>>> 
>>> * Enhancing narrowing reference conversion to allow for stricter 
>>> checking of cast conversions with respect to sealed type hierarchies
>>> 
>>> * Also local classes are not considered when determining implicitly 
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>> 
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>>> still in the same method, the return type has been changed to Class[] 
>>> instead of the previous ClassDesc[]
>>> 
>>> * adding code to make sure that annotations can't be sealed
>>> 
>>> * improving some tests
>>> 
>>> 
>>> TIA
>>> 
>>> Related specs:
>>> [Sealed Classes 
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes 
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual 
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>> 
>> This PR strives to reflect the review comments from 1227:
>>  * adjustments to javadoc of j.l.Class methods
>>  * package access checks in Class.getPermittedSubclasses()
>>  * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

Marked as reviewed by alanb (Reviewer).

src/java.base/share/classes/java/lang/Class.java line 3043:

> 3041: for (Class c : subClasses) {
> 3042: if (Proxy.isProxyClass(c))
> 3043: throw new InternalError("a permitted subclass 
> should not be a proxy class: " + c);

Minor nit but I think the indentation may be messed up here.

src/java.base/share/classes/java/lang/Class.java line 4394:

> 4392:  * implement this class or interface if it is sealed. The order of 
> such elements
> 4393:  * is unspecified. If this {@code Class} object represents a 
> primitive type,
> 4394:  * {@code void}, an array type, or a class or interface that is not 
> sealed,

Did you mean {@code Void} here?

src/java.base/share/classes/java/lang/Class.java line 4403:

> 4401:  * loader} of the current {@code Class} object).
> 4402:  * The {@code Class} objects which can be obtained using this 
> procedure,
> 4403:  * and which are direct subinterfaces or subclasses of this class 
> or interface,

Minor suggestion is to drop "using this procedure" from this sentence.

-

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

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v5]

[Jan Lahoda]

> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
> 
> From the original PR:
> 
>> Please review the code for the second iteration of sealed classes. In this 
>> iteration we are:
>> 
>> * Enhancing narrowing reference conversion to allow for stricter 
>> checking of cast conversions with respect to sealed type hierarchies
>> 
>> * Also local classes are not considered when determining implicitly 
>> declared permitted direct subclasses of a sealed class or sealed interface
>> 
>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, 
>> still in the same method, the return type has been changed to Class[] 
>> instead of the previous ClassDesc[]
>> 
>> * adding code to make sure that annotations can't be sealed
>> 
>> * improving some tests
>> 
>> 
>> TIA
>> 
>> Related specs:
>> [Sealed Classes 
>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>> [Sealed Classes 
>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>> [Additional: Contextual 
>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
> 
> This PR strives to reflect the review comments from 1227:
>  * adjustments to javadoc of j.l.Class methods
>  * package access checks in Class.getPermittedSubclasses()
>  * fixed to the narrowing conversion/castability as pointed out by Maurizio

Jan Lahoda has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Cleanup as suggested on the review.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1483/files
  - new: https://git.openjdk.java.net/jdk/pull/1483/files/537e267f..09e0c186

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

  Stats: 187 lines in 7 files changed: 38 ins; 114 del; 35 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1483.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1483/head:pull/1483

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Alan Bateman]

On Fri, 4 Dec 2020 11:38:51 GMT, Magnus Ihse Bursie  wrote:

> And I can certainly move jdwp.spec to java.base instead. 

If jdwp.spec has to move to the src tree then src/java.se is probably the most 
suitable home because Java SE specifies JDWP as an optional interface. So 
nothing to do with java.base and the build will need to continue to generate 
the sources for the front-end (jdk.jdi) and back-end (jdk.jdwp.agent) as they 
implement the protocol.

-

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

Integrated: 8257622: MemoryAccess methods are missing @ForceInline annotations

[Maurizio Cimadamore]

On Wed, 2 Dec 2020 18:47:10 GMT, Maurizio Cimadamore  
wrote:

> The accessor methods in the `MemoryAccess` class are missing `@ForceInline` 
> annotations. This causes odd behavior on certain benchmarks, especially if 
> these methods are called many times in the body of a single method.

This pull request has now been integrated.

Changeset: dede01eb
Author:Maurizio Cimadamore 
URL:   https://git.openjdk.java.net/jdk/commit/dede01eb
Stats: 325 lines in 3 files changed: 325 ins; 0 del; 0 mod

8257622: MemoryAccess methods are missing @ForceInline annotations

Reviewed-by: jvernee, shade

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 11:14:49 GMT, Alan Bateman  wrote:

>> To facilitate review, here is a list on how the different directories under 
>> make/data has moved.
>> 
>> **To java.base:**
>> * blacklistedcertsconverter
>> * cacerts
>> * characterdata
>> * charsetmapping
>> * cldr
>> * currency
>> * lsrdata
>> * publicsuffixlist
>> * tzdata
>> * unicodedata
>> 
>> **To java.desktop:**
>> * dtdbuilder
>> * fontconfig
>> * macosxicons
>> * x11wrappergen
>> 
>> **To jdk.compiler:**
>> * symbols
>> 
>> **To jdk.jdi:**
>> * jdwp
>> 
>> **Remaining in make:**
>> * bundle
>> * docs-resources
>> * macosxsigning
>> * mainmanifest
>
> Are you proposing any text or guidelines to be added to JEP 201 as part of 
> this?
> 
> I think the location of jdwp.spec and its location in the build tree may need 
> to be looked at again. It was convenient to have it in the make tree, from 
> which the protocol spec, and source code for the front end (module jdk.jdi) 
> and a header file for the back end (module jdk.jdwp.agent) are created. Given 
> that the JDWP protocol is standard (not JDK-specific) then there may be an 
> argument to put it in src/java.se instead of a JDK-specific module.

@AlanBateman Well, I don't know about updating JEP 201. Do you mean that `data` 
should be added to the list `classes`, `native`, `conf`, `legal` as presented 
under the heading "New scheme"? That list does not seem to have been kept up to 
date anyway. A quick glance also shows that we now have at least `man` and 
`lib` as well in this place. So either we say there's precedence for not 
updating the list, in which case I will do nothing. Or we should bring JEP 201 
up-to-date with current practices, which then of course should include `data` 
but also all other new directories that has been added since JEP 201 was 
written.

I really don't care either way, but my personal opinion is that JEP 201 
presented a view on how the plan was to re-organize things, given the knowledge 
and state of affairs at that time, but how we keep the source code organized 
and structured from there on, is a living, day-to-day engineering effort that 
is just hampered by having to update a formal document, that serves little 
purpose now that it has been implemented.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 11:37:41 GMT, Magnus Ihse Bursie  wrote:

>> Are you proposing any text or guidelines to be added to JEP 201 as part of 
>> this?
>> 
>> I think the location of jdwp.spec and its location in the build tree may 
>> need to be looked at again. It was convenient to have it in the make tree, 
>> from which the protocol spec, and source code for the front end (module 
>> jdk.jdi) and a header file for the back end (module jdk.jdwp.agent) are 
>> created. Given that the JDWP protocol is standard (not JDK-specific) then 
>> there may be an argument to put it in src/java.se instead of a JDK-specific 
>> module.
>
> @AlanBateman Well, I don't know about updating JEP 201. Do you mean that 
> `data` should be added to the list `classes`, `native`, `conf`, `legal` as 
> presented under the heading "New scheme"? That list does not seem to have 
> been kept up to date anyway. A quick glance also shows that we now have at 
> least `man` and `lib` as well in this place. So either we say there's 
> precedence for not updating the list, in which case I will do nothing. Or we 
> should bring JEP 201 up-to-date with current practices, which then of course 
> should include `data` but also all other new directories that has been added 
> since JEP 201 was written.
> 
> I really don't care either way, but my personal opinion is that JEP 201 
> presented a view on how the plan was to re-organize things, given the 
> knowledge and state of affairs at that time, but how we keep the source code 
> organized and structured from there on, is a living, day-to-day engineering 
> effort that is just hampered by having to update a formal document, that 
> serves little purpose now that it has been implemented.

And I can certainly move jdwp.spec to java.base instead. That's the reason I 
need input on this: All I know is that is definitely not the responsibility of 
the Build Group to maintain that document, and I made my best guess at where to 
place it.

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Alan Bateman]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Good to use this done in the same release as refersTo was added.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Alan Bateman]

On Fri, 4 Dec 2020 10:29:48 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>
> To facilitate review, here is a list on how the different directories under 
> make/data has moved.
> 
> **To java.base:**
> * blacklistedcertsconverter
> * cacerts
> * characterdata
> * charsetmapping
> * cldr
> * currency
> * lsrdata
> * publicsuffixlist
> * tzdata
> * unicodedata
> 
> **To java.desktop:**
> * dtdbuilder
> * fontconfig
> * macosxicons
> * x11wrappergen
> 
> **To jdk.compiler:**
> * symbols
> 
> **To jdk.jdi:**
> * jdwp
> 
> **Remaining in make:**
> * bundle
> * docs-resources
> * macosxsigning
> * mainmanifest

Are you proposing any text or guidelines to be added to JEP 201 as part of this?

I think the location of jdwp.spec and its location in the build tree may need 
to be looked at again. It was convenient to have it in the make tree, from 
which the protocol spec, and source code for the front end (module jdk.jdi) and 
a header file for the back end (module jdk.jdwp.agent) are created. Given that 
the JDWP protocol is standard (not JDK-specific) then there may be an argument 
to put it in src/java.se instead of a JDK-specific module.

-

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

Re: RFR: 8257622: MemoryAccess methods are missing @ForceInline annotations [v2]

[Maurizio Cimadamore]

> The accessor methods in the `MemoryAccess` class are missing `@ForceInline` 
> annotations. This causes odd behavior on certain benchmarks, especially if 
> these methods are called many times in the body of a single method.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix extra whitespace in benchmark

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1570/files
  - new: https://git.openjdk.java.net/jdk/pull/1570/files/b2965389..737af864

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

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

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Daniel Fuchs]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Hi Mandy,

This looks good to me. There are a few places where a single call to 
`Reference::get` is replaced by multiple calls to `Reference::refersTo`, 
allowing the reference to get cleared in between, but as far as I could see 
that doesn't affect the overall logic which is still sound.
So LGTM!

best regards,
-- daniel

-

Marked as reviewed by dfuchs (Reviewer).

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Thu, 3 Dec 2020 23:44:20 GMT, Magnus Ihse Bursie  wrote:

> A lot (but not all) of the data in make/data is tied to a specific module. 
> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
> make for the whole build.) 
> 
> These data files should move to the module they belong to. The are, after 
> all, "source code" for that module that is "compiler" into resulting 
> deliverables, for that module. (But the "source code" language is not Java or 
> C, but typically a highly domain specific language or data format, and the 
> "compilation" is, often, a specialized transformation.) 
> 
> This misplacement of the data directory is most visible at code review time. 
> When such data is changed, most of the time build-dev (or the new build 
> label) is involved, even though this has nothing to do with the build. While 
> this is annoying, a worse problem is if the actual team that needs to review 
> the patch (i.e., the team owning the module) is missed in the review.

To facilitate review, here is a list on how the different directories under 
make/data has moved.

**To java.base:**
* blacklistedcertsconverter
* cacerts
* characterdata
* charsetmapping
* cldr
* currency
* lsrdata
* publicsuffixlist
* tzdata
* unicodedata

**To java.desktop:**
* dtdbuilder
* fontconfig
* macosxicons
* x11wrappergen

**To jdk.compiler:**
* symbols

**To jdk.jdi:**
* jdwp

**Remaining in make:**
* bundle
* docs-resources
* macosxsigning
* mainmanifest

-

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

RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

A lot (but not all) of the data in make/data is tied to a specific module. For 
instance, the publicsuffixlist is used by java.base, and fontconfig by 
java.desktop. (A few directories, like mainmanifest, is *actually* used by make 
for the whole build.) 

These data files should move to the module they belong to. The are, after all, 
"source code" for that module that is "compiler" into resulting deliverables, 
for that module. (But the "source code" language is not Java or C, but 
typically a highly domain specific language or data format, and the 
"compilation" is, often, a specialized transformation.) 

This misplacement of the data directory is most visible at code review time. 
When such data is changed, most of the time build-dev (or the new build label) 
is involved, even though this has nothing to do with the build. While this is 
annoying, a worse problem is if the actual team that needs to review the patch 
(i.e., the team owning the module) is missed in the review.

-

Commit messages:
 - Update references in test
 - Step 2: Update references
 - First stage, move actual data files

Changes: https://git.openjdk.java.net/jdk/pull/1611/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1611=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257733
  Stats: 78 lines in 1575 files changed: 3 ins; 1 del; 74 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1611.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1611/head:pull/1611

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

Re: RFR: 8251989: Hex formatting and parsing utility [v17]

[Daniel Fuchs]

On Wed, 2 Dec 2020 16:33:15 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat 
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> 
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add class level clarification of use of uppercase for primitive conversions
>   Switched order of declaration of a couple of method to make the javadoc 
> sequence easier to read

Marked as reviewed by dfuchs (Reviewer).

-

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

Re: RFR: 8251989: Hex formatting and parsing utility [v17]

[Chris Hegarty]

On Wed, 2 Dec 2020 16:33:15 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat 
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> 
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add class level clarification of use of uppercase for primitive conversions
>   Switched order of declaration of a couple of method to make the javadoc 
> sequence easier to read

Marked as reviewed by chegar (Reviewer).

-

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

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Aleksey Shipilev]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Replacements look fine to me.

src/java.base/share/classes/java/util/WeakHashMap.java line 291:

> 289: if (e.refersTo(key)) return true;
> 290: 
> 291: // then checks for equality

Obnoxiously minor nit: plurality is inconsistent. `check if the given entry...` 
above, and `then check[s] for equality`. Should be `...then check for equality`?

-

Marked as reviewed by shade (Reviewer).

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

Integrated: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored

[Alan Bateman]

On Tue, 24 Nov 2020 10:58:43 GMT, Alan Bateman  wrote:

> The attribute_length of known Module attributes in the module-info.class 
> is currently ignored. It should be checked and the class rejected if the 
> attribute length doesn't exactly match the length of the info in the 
> attribute.
> 
> There are several ways to fix this. I initially limited the reading of the 
> attribute_info to the attribute length but this resulted in confusing 
> exception messages as the attribute appears truncated. The exception messages 
> are clearer when it checks that the attribute length corresponds to the 
> number of bytes read.

This pull request has now been integrated.

Changeset: 2b4a423f
Author:Alan Bateman 
URL:   https://git.openjdk.java.net/jdk/commit/2b4a423f
Stats: 567 lines in 8 files changed: 562 ins; 1 del; 4 mod

8255542: Attribute length of Module, ModulePackages and other attributes is 
ignored

Reviewed-by: mchung, dfuchs, chegar

-

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