Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
367 /** 368 * called from vm to generate MethodHandle holder classes 369
* @return @code { Object[] } if holder classes can be generated. 370 *
@param lines the output lines from @code { VM.cdsTraceResolve } 371 */
@code {} is wrong syntax. It should be {@code } 372 static
Object[] cdsGenerateHolderClasses(String[] lines) { this can be made
private as it's invoked by VM only. Can you move it to the end of the
file. 373 try { 374 Map result =
generateHolderClasses(Arrays.stream(lines)); 375 if (result == null) {
376 return null; 377 }
generateHolderClasses should never return null and so line 371-377 can
be dropped. I also suggest to add in the generateHolderClasses method
to add Objects.requireNonNull(traces).
379 Object[] ret_array = new Object[size * 2];
Rename `ret_array` to retArray to follow the naming convention using camel case.
src/java.base/share/classes/jdk/internal/misc/VM.java
457 isDumpLoadedClassListSetAndOpen =
isDumpLoadedClassListSetAndOpen0();
This should be cached in the caller who checks if -XX:+DumpLoadedClassList
instead of every VM initialization.
Since there are many CDS-related methods, I think it's time to introduce
a new CDS class for these methods to reside (maybe jdk.internal.vm.CDS?).
I suggest to add CDS:logTraceResolve(String line) method that
will check if isDumpLoadedClassListSetAndOpen is true, then call the
native cdsTraceResolve (or whatever name). This way,
GenerateJLIClassesHelper can simply call CDS::logTraceResolve.
493 * Output to DumpLoadedClassList, format is simimar to LF_RESOLVE
s/simimar/similar
494 * @see InvokerBytecodeGenerator
495 * @param line the line to output.
@see is typically placed after @param.
Should it say @see java.lang.invoke.GenerateJLIClassesHelper::traceLambdaForm
instead of InvokerBytecodeGenerator?
Mandy
On 9/15/20 12:15 PM, Yumin Qi wrote:
This patch is reorganized after 8252725, which is separated from this patch to
refactor jlink glugin code. The previous
webrev with hg can be found at:
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
integrated, the
regeneration of holder classes is simply to call the new added
GenerateJLIClassesHelper.cdsGenerateHolderClasses
function.
Tests: tier1-4
-
Commit messages:
- 8247536: Support for pre-generated java.lang.invoke classes in CDS static
archive
- Merge remote-tracking branch 'upstream/master' into jdk-8247536
- Merge remote-tracking branch 'upstream/master' into jdk-8247536
- Merge remote-tracking branch 'origin/jdk-8252689'
- 8252689: Classes are loaded from jrt:/java.base even when CDS is used
- 8252689: Classes are loaded from jrt:/java.base even when CDS is used
- 8252689: Classes are loaded from jrt:/java.base even when CDS is used
Changes: https://git.openjdk.java.net/jdk/pull/193/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=193=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8247536
Stats: 368 lines in 18 files changed: 344 ins; 13 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/193.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193
PR: https://git.openjdk.java.net/jdk/pull/193
Integrated: 8220483: Calendar.setTime(Date date) throws NPE with Date date = null
On Mon, 14 Sep 2020 22:18:34 GMT, Naoto Sato wrote: > Hi, > > Please review this simple doc fix. This pull request has now been integrated. Changeset: 57f92d23 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/57f92d23 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8220483: Calendar.setTime(Date date) throws NPE with Date date = null Reviewed-by: lancea, joehw - PR: https://git.openjdk.java.net/jdk/pull/159
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
Note that we've had the javac warning about equals and hashCode overriding as a fatal build condition since at least JDK 9. -Joe On 9/15/2020 12:36 PM, Brian Burkhalter wrote: Hello, The override of hashCode() looks like it is still there in JDK 15 [1]. I don’t know however why it does not appear as such in the javadoc [2]. Brian [1] http://hg.openjdk.java.net/jdk/jdk15/file/fb7064dc63f9/src/java.base/share/classes/java/text/DecimalFormatSymbols.java#l760 [2] https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/text/DecimalFormatSymbols.html On Sep 15, 2020, at 12:14 PM, Rob Spoor wrote: In Java 14 and before, DecimalFormatSymbols had overrides for both equals and hashCode. In Java 15, the override for hashCode has disappeared, and it now inherits hashCode from java.lang.Object. That means it now violates the contract for equals + hashCode: two equal DecimalFormatSymbols instances can have different hash codes.
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
On 15/09/2020 22:02, Pavel Rappo wrote:
On 15 Sep 2020, at 20:50, Brian Burkhalter wrote:
On Sep 15, 2020, at 12:38 PM, Kevin Rushforth
wrote:
I see this in DecimalFormatSymbols:
/**
* Override hashCode.
*/
private volatile int hashCode;
@Override
public int hashCode() {
Although, I'm not sure why the intervening private field would prevent javadoc
from generating at least a method with an empty doc
https://bugs.openjdk.java.net/browse/JDK-8187386
So in this case, the solution would be to remove the superfluous
"Override equals." comment from the equals method, right?
RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
HI, all Please review changes for 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive. What happened: I pushed with commit comment: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive When created pullrequest, the title is jdk 8247536, I did not pay attention to it, thought it will be filled correct. But it wasn't. The jcheck failed on checking the title, after I modified the title to correct form, there are no emails sent out, so I send here the link, please review via this link: https://github.com/openjdk/jdk/pull/193/files Thanks Yumin
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 16:43:01 GMT, Daniil Titov wrote:
> The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes()
> when the current thread is in the middle
> of redefining the same class. The change is based on the fix [1] suggested in
> the Jira issue [2] comment.
> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/
> [2] https://bugs.openjdk.java.net/browse/JDK-8241390
>
> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass.
Changes requested by coleenp (Reviewer).
test/jdk/java/lang/instrument/MakeAgentJAR.sh line 1:
> 1: #!/bin/sh
There are tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses that
don't use shell scripts that are much
better. Can you add this test using that framework instead?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 159:
> 157: if (!cls->contains(def_ik)) {
> 158: def_ik->set_is_being_redefined(false);
> 159: }
Ok, so adding the Klass to the thread local list for each recursion works like
ref counting. Can't think of a simpler
way to do this. Looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/190
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
> On 15 Sep 2020, at 20:50, Brian Burkhalter
> wrote:
>
>> On Sep 15, 2020, at 12:38 PM, Kevin Rushforth
>> wrote:
>>
>> I see this in DecimalFormatSymbols:
>>
>>
>> /**
>> * Override hashCode.
>> */
> private volatile int hashCode;
>> @Override
>> public int hashCode() {
>>
>> Although, I'm not sure why the intervening private field would prevent
>> javadoc from generating at least a method with an empty doc
https://bugs.openjdk.java.net/browse/JDK-8187386
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 16:59:59 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > I am fine with this as well. I will pull over the change and just sanity > check it via mach5 and then will sponsor @jaikiran Please go ahead and integrate this and I can then sponsor (has to be done in that order) - PR: https://git.openjdk.java.net/jdk/pull/186
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
> On Sep 15, 2020, at 12:38 PM, Kevin Rushforth
> wrote:
>
> I see this in DecimalFormatSymbols:
>
>
> /**
> * Override hashCode.
> */
> >>>private volatile int hashCode;
> @Override
> public int hashCode() {
>
> Although, I'm not sure why the intervening private field would prevent
> javadoc from generating at least a method with an empty doc.
Moving the hashCode private field before the comment block causes the javadoc
to be generated as expected.
--- a/src/java.base/share/classes/java/text/DecimalFormatSymbols.java
+++ b/src/java.base/share/classes/java/text/DecimalFormatSymbols.java
@@ -757,10 +757,11 @@
locale.equals(other.locale));
}
+private volatile int hashCode;
+
/**
* Override hashCode.
*/
-private volatile int hashCode;
@Override
public int hashCode() {
if (hashCode == 0) {
> On Sep 15, 2020, at 12:43 PM, Rob Spoor wrote:
>
> Hmm, I could have sworn I checked
> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/text/DecimalFormatSymbols.java__;!!GqivPVa7Brio!JJ8ymM95_Eg-fb-IIWGHHAvXTmKEX7h9-vSxyH73DxsjaS_0-bNOJ96Ud_1LMFCR1G0m$
>
>
> and didn't find it...
>
> I guess it's a false positive then.
In the sense that the hsdhCode() method is not in fact removed, but it is a bug
that the javadoc is not generated as one would expect.
Thanks,
Brian
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
Hmm, I could have sworn I checked https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/text/DecimalFormatSymbols.java and didn't find it... I guess it's a false positive then. On 15/09/2020 21:36, Brian Burkhalter wrote: Hello, The override of hashCode() looks like it is still there in JDK 15 [1]. I don’t know however why it does not appear as such in the javadoc [2]. Brian [1] http://hg.openjdk.java.net/jdk/jdk15/file/fb7064dc63f9/src/java.base/share/classes/java/text/DecimalFormatSymbols.java#l760 [2] https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/text/DecimalFormatSymbols.html On Sep 15, 2020, at 12:14 PM, Rob Spoor wrote: In Java 14 and before, DecimalFormatSymbols had overrides for both equals and hashCode. In Java 15, the override for hashCode has disappeared, and it now inherits hashCode from java.lang.Object. That means it now violates the contract for equals + hashCode: two equal DecimalFormatSymbols instances can have different hash codes.
Re: RFR: 8250859: Address reliance on default constructors in the Accessibility APIs
On Tue, 15 Sep 2020 10:04:49 GMT, Conor Cleary wrote: > This issue relates to JDK-8250639 '☂ Address reliance on default constructors > in the java.desktop module'. The > following classes have had an explicit no-arg constructor added, with a > protected access modifier and accompanying API > description: > - Default ctor on com.sun.java.accessibility.util.SwingEventMonitor > - Default ctor on javax.accessibility.AccessibleContext > - Default ctor on javax.accessibility.AccessibleHyperlink > > The following classes have had an explicit no-arg constructor added, with a > public access modifier and accompanying API > description: > - Default ctor on javax.accessibility.AccessibleResourceBundle > - Default ctor on com.sun.java.accessibility.util.AWTEventMonitor > - Default ctor on com.sun.java.accessibility.util.AccessibilityEventMonitor > - Default ctor on com.sun.java.accessibility.util.AccessibilityListenerList > - Default ctor on com.sun.java.accessibility.util.EventID > > specdiff: > http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8250859/webrevs/webrev.00/specdiff/overview-summary.html > bug: > https://bugs.openjdk.java.net/browse/JDK-8250859 This looks OK but the CSR needs updates first. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/174
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
I see this in DecimalFormatSymbols:
/**
* Override hashCode.
*/
>>> private volatile int hashCode;
@Override
public int hashCode() {
Although, I'm not sure why the intervening private field would prevent
javadoc from generating at least a method with an empty doc.
-- Kevin
On 9/15/2020 12:36 PM, Brian Burkhalter wrote:
Hello,
The override of hashCode() looks like it is still there in JDK 15 [1]. I don’t
know however why it does not appear as such in the javadoc [2].
Brian
[1]
http://hg.openjdk.java.net/jdk/jdk15/file/fb7064dc63f9/src/java.base/share/classes/java/text/DecimalFormatSymbols.java#l760
[2]
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/text/DecimalFormatSymbols.html
On Sep 15, 2020, at 12:14 PM, Rob Spoor wrote:
In Java 14 and before, DecimalFormatSymbols had overrides for both equals and
hashCode. In Java 15, the override for hashCode has disappeared, and it now
inherits hashCode from java.lang.Object. That means it now violates the
contract for equals + hashCode: two equal DecimalFormatSymbols instances can
have different hash codes.
Re: [BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
Hello, The override of hashCode() looks like it is still there in JDK 15 [1]. I don’t know however why it does not appear as such in the javadoc [2]. Brian [1] http://hg.openjdk.java.net/jdk/jdk15/file/fb7064dc63f9/src/java.base/share/classes/java/text/DecimalFormatSymbols.java#l760 [2] https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/text/DecimalFormatSymbols.html > On Sep 15, 2020, at 12:14 PM, Rob Spoor wrote: > > In Java 14 and before, DecimalFormatSymbols had overrides for both equals and > hashCode. In Java 15, the override for hashCode has disappeared, and it now > inherits hashCode from java.lang.Object. That means it now violates the > contract for equals + hashCode: two equal DecimalFormatSymbols instances can > have different hash codes.
Re: RFR: 8252537: Updated @exception with @throws
On Tue, 15 Sep 2020 19:13:55 GMT, Vipin Sharma wrote:
>> I've only looked at the management files. They look good in general.
>>
>> src/java.management/share/classes/java/lang/management/ClassLoadingMXBean.java
>>
>> 108 * @throws java.lang.SecurityException if a security manager
>> 109 * exists and the caller does not have
>> 110 * ManagementPermission("control").
>>
>>
>> src/java.management/share/classes/java/lang/management/MemoryMXBean.java
>>
>> 286 * @throws java.lang.SecurityException if a security manager
>> 287 * exists and the caller does not have
>> 288 * ManagementPermission("control").
>>
>> Could you, please, fix the indentation?
>
> @RogerRiggs I understand your point and will update PR with correct
> indentation.
> But I think adding 3 spaces after throws may not be right for all cases.
> For example when
> 1. Another tag in same method is using only 1 space.
> 2. In some cases (e.g. free method of Blob.java) we had a mix of throws and
> exception in the same method both with one
> space after. Here after adding 3 spaces throws tags will have the different
> number of spaces and indentation will not
> be same as before. I will update PR to make sure the indentation looks same
> as before and there is no change in
> javadoc. Please tell me in case my understnding is not correct here.
HI Vipin,
Correct, a better description is "fix the indentation".
I mnetioned 3 because that was the difference in length between
"exception" and "throws".
Thanks for the followup, Roger
On 9/15/20 3:14 PM, Vipin Sharma wrote:
>
> @RogerRiggs
>
> I understand your point and will update PR with correct indentation.
> But I think adding 3 spaces after throws may not be right for all cases.
> For example when
>
> 1. Another tag in same method is using only 1 space.
> 2. In some cases (e.g. free method of Blob.java) we had a mix of
> throws and exception in the same method both with one space after.
> Here after adding 3 spaces throws tags will have the different
> number of spaces and indentation will not be same as before.
>
> I will update PR to make sure the indentation looks same as before and
> there is no change in javadoc.
> Please tell me in case my understnding is not correct here.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> .
>
-
PR: https://git.openjdk.java.net/jdk/pull/95
Re: RFR: 8252537: Updated @exception with @throws
On Tue, 15 Sep 2020 18:14:58 GMT, Serguei Spitsyn wrote:
>> Updated @exception with @throws for core-libs, it fixes all open sub-tasks
>> of JDK-8252536.
>>
>> Open Subtasks part of this fix are:
>> 1. JDK-8252537
>> 2. JDK-8252539
>> 3. JDK-8252540
>> 4. JDK-8252541
>>
>> Previous conversation on this:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068540.html
>
> I've only looked at the management files. They look good in general.
>
> src/java.management/share/classes/java/lang/management/ClassLoadingMXBean.java
>
> 108 * @throws java.lang.SecurityException if a security manager
> 109 * exists and the caller does not have
> 110 * ManagementPermission("control").
>
>
> src/java.management/share/classes/java/lang/management/MemoryMXBean.java
>
> 286 * @throws java.lang.SecurityException if a security manager
> 287 * exists and the caller does not have
> 288 * ManagementPermission("control").
>
> Could you, please, fix the indentation?
@RogerRiggs I understand your point and will update PR with correct indentation.
But I think adding 3 spaces after throws may not be right for all cases.
For example when
1. Another tag in same method is using only 1 space.
2. In some cases (e.g. free method of Blob.java) we had a mix of throws and
exception in the same method both with one
space after. Here after adding 3 spaces throws tags will have the different
number of spaces and indentation will not
be same as before.
I will update PR to make sure the indentation looks same as before and there is
no change in javadoc.
Please tell me in case my understnding is not correct here.
-
PR: https://git.openjdk.java.net/jdk/pull/95
[BUG] Java 15: DecimalFormatSymbols overrides equals but not hashCode
In Java 14 and before, DecimalFormatSymbols had overrides for both equals and hashCode. In Java 15, the override for hashCode has disappeared, and it now inherits hashCode from java.lang.Object. That means it now violates the contract for equals + hashCode: two equal DecimalFormatSymbols instances can have different hash codes.
RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
This patch is reorganized after 8252725, which is separated from this patch to refactor jlink glugin code. The previous webrev with hg can be found at: http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 integrated, the regeneration of holder classes is simply to call the new added GenerateJLIClassesHelper.cdsGenerateHolderClasses function. Tests: tier1-4 - Commit messages: - 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive - Merge remote-tracking branch 'upstream/master' into jdk-8247536 - Merge remote-tracking branch 'upstream/master' into jdk-8247536 - Merge remote-tracking branch 'origin/jdk-8252689' - 8252689: Classes are loaded from jrt:/java.base even when CDS is used - 8252689: Classes are loaded from jrt:/java.base even when CDS is used - 8252689: Classes are loaded from jrt:/java.base even when CDS is used Changes: https://git.openjdk.java.net/jdk/pull/193/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=193=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247536 Stats: 368 lines in 18 files changed: 344 ins; 13 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v4]
> Related to [JDK-8252730 jlink does not create reproducible builds on different > servers](https://bugs.openjdk.java.net/browse/JDK-8252730). Introduces > ordering based on `Archive` module names to > ensure stable files (and file signatures) across generated JDK images by > jlink. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Test updates - Changes: - all: https://git.openjdk.java.net/jdk/pull/156/files - new: https://git.openjdk.java.net/jdk/pull/156/files/b89d2c4d..860c6096 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=156=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=156=02-03 Stats: 59 lines in 2 files changed: 58 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/156.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/156/head:pull/156 PR: https://git.openjdk.java.net/jdk/pull/156
Re: RFR: 8252537: Updated @exception with @throws
On Wed, 9 Sep 2020 19:29:30 GMT, Vipin Sharma wrote:
> Updated @exception with @throws for core-libs, it fixes all open sub-tasks of
> JDK-8252536.
>
> Open Subtasks part of this fix are:
> 1. JDK-8252537
> 2. JDK-8252539
> 3. JDK-8252540
> 4. JDK-8252541
>
> Previous conversation on this:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068540.html
I've only looked at the management files. They look good in general.
src/java.management/share/classes/java/lang/management/ClassLoadingMXBean.java
108 * @throws java.lang.SecurityException if a security manager
109 * exists and the caller does not have
110 * ManagementPermission("control").
src/java.management/share/classes/java/lang/management/MemoryMXBean.java
286 * @throws java.lang.SecurityException if a security manager
287 * exists and the caller does not have
288 * ManagementPermission("control").
Could you, please, fix the indentation?
-
Changes requested by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/95
'Find' method for Iterable
Hi,
This has probably been brought up at some point. When we need to find an
item in a collection based on its properties, we can either do it in a
loop, testing each item, or in a stream with filter and findFirst/Any.
I would think that a method in Iterable be useful, along the lines of:
public Optional find(Predicate condition) {
Objects.requireNonNull(condition);
for (T t : this) {
if (condition.test(t)) {
return Optional.of(t);
}
}
return Optional.empty();
}
With usage:
list.find(person -> person.id == 123456);
There are a few issues with the method here such as t being null in
null-friendly collections and the lack of bound generic types, but this
example is just used to explain the intention.
It will be an alternative to
list.stream().filter(person -> person.id == 123456).findAny/First()
(depending on if the collection is ordered or not)
which doesn't create a stream, similar to Iterable#forEach vs
Stream#forEach.
Maybe with pattern matching this would become more appetizing.
- Nir
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v4]
On Tue, 15 Sep 2020 12:39:11 GMT, Jaikiran Pai wrote: >> Can I please get a review and a sponsor for this patch which fixes the issue >> reported in >> https://bugs.openjdk.java.net/browse/JDK-8244706? >> The commit here sets the `OS` header flag to `255` (which represents >> `unknown`) as noted in [1]. A new test has been >> included in this commit to verify the change. Furthermore, this doesn't >> impact the `java.util.zip.GZIPInputStream` >> since it ignores [2] this header value while reading the headers from the >> stream. [1] >> https://tools.ietf.org/html/rfc1952#page-7 [2] >> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173 > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unintended file commit Thank you for making the changes that Brent and I suggested. This looks good. I have submitted, a [CSR](https://bugs.openjdk.java.net/browse/JDK-8253142) to track the change. I have also created a [Release Note](https://bugs.openjdk.java.net/browse/JDK-8253185) as well. Once the CSR has been approved, I will sponsor your change - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/130
RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() when the current thread is in the middle of redefining the same class. The change is based on the fix [1] suggested in the Jira issue [2] comment. [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8241390 Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. - Commit messages: - 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() Changes: https://git.openjdk.java.net/jdk/pull/190/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=190=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8241390 Stats: 202 lines in 6 files changed: 193 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/190/head:pull/190 PR: https://git.openjdk.java.net/jdk/pull/190
Integrated: 8251495: Remove the implNote in the DOM package description added by JDK-8249643
On Wed, 9 Sep 2020 22:56:14 GMT, Joe Wang wrote: > Revert changes made by JDK-8249643, removing the implNote. This pull request has now been integrated. Changeset: 5191f315 Author:Joe Wang URL: https://git.openjdk.java.net/jdk/commit/5191f315 Stats: 25 lines in 1 file changed: 25 ins; 0 del; 0 mod 8251495: Remove the implNote in the DOM package description added by JDK-8249643 Remove the implNote from the package description added by JDK-8249643. Reviewed-by: lancea, smarks, naoto, alanb - PR: https://git.openjdk.java.net/jdk/pull/100
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v4]
On Tue, 15 Sep 2020 12:39:11 GMT, Jaikiran Pai wrote: >> Can I please get a review and a sponsor for this patch which fixes the issue >> reported in >> https://bugs.openjdk.java.net/browse/JDK-8244706? >> The commit here sets the `OS` header flag to `255` (which represents >> `unknown`) as noted in [1]. A new test has been >> included in this commit to verify the change. Furthermore, this doesn't >> impact the `java.util.zip.GZIPInputStream` >> since it ignores [2] this header value while reading the headers from the >> stream. [1] >> https://tools.ietf.org/html/rfc1952#page-7 [2] >> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173 > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unintended file commit Reviewed. Thanks for reworking the change into the existing out.write() call. Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/130
Re: RFR: 8202473: A type variable with multiple bounds does not correctly place type annotation
On Tue, 15 Sep 2020 11:31:58 GMT, Joel Borggrén-Franck wrote: >> Pre-git review thread here: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067049.html > > @raphw test is missing imports and doesn't compile, other than that looks > good. Bad copy paste. Fixed on the branch. - PR: https://git.openjdk.java.net/jdk/pull/155
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > As for this: > >> As long as the input stream close() method is idem potent this should be >> safe, and AFAICS that is the case for the two >> input stream subclasses that can be returned by ZipFile::getInputStream. > > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this InputStream instance? I think I understand what you meant. You were perhaps talking about the `JarFile.close` triggering the `Cleanable` to close this `InputStream` in addition to the `try-with-resources` already calling `close` on that stream. Like you rightly note, the implementation of those streams already handles that aspect correctly. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this `InputStream` instance? My bad - I hadn't realised closing the input stream would also remove it from the Cleanable resource set, so I thought it might be closed again when the jar file is closed. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:33:51 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > Hi Jaikiran, > > This is not an area I know too well - so I won't review formally, but the > proposed changes look reasonable to me. > Closing the stream from within `JarFile` after creating the manifest looks > innocuous and should release any resource > held by the stream earlier instead of waiting for the `JarFile` to be closed. > As long as the input stream `close()` > method is idem potent this should be safe, and AFAICS that is the case for > the two input stream subclasses that can be > returned by `ZipFile::getInputStream`. WRT to the claims in the JBS issue I > see that that was logged against Java 6: > there was no `Cleanable` at this time and it is possible that the internals > of ZipFile/JarFile were quite different. Thank you for the review Daniel. > WRT to the claims in the JBS issue I see that that was logged against Java 6: > there was no Cleanable at this time and > it is possible that the internals of ZipFile/JarFile were quite different. You are right. I hadn't paid attention to that detail. It's likely that it might have been behaving differently at that time. As for this: > As long as the input stream close() method is idem potent this should be > safe, and AFAICS that is the case for the two > input stream subclasses that can be returned by ZipFile::getInputStream. I'm curious, in the context of this change, why idempotency would be a necessity. Would there be a "double close" somehow on this `InputStream` instance? - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:29:44 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove "final"
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 428:
>
>> 426: try (final InputStream is =
>> super.getInputStream(manEntry)) {
>> 427: man = new Manifest(is, getName());
>> 428: }
>
> There is a cleaner so shouldn't have a leak, even if the JarFile is not
> explicitly closed.
> The noisy "final" can be dropped, otherwise looks good.
Thank you for the review Alan. I've updated this PR to remove the `final`. And
yes as you note, this doesn't really
leak. This change closes the InputStream earlier, as soon as it is done,
instead of waiting for the `Cleaner` to kick
in.
-
PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 8202473: A type variable with multiple bounds does not correctly place type annotation [v2]
> This patch assures that an annotation on a type bound is placed on the > correct bound index. Rafael Winterhalter 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: 8202473: A type variable with multiple bounds does not correctly place type annotation - Changes: - all: https://git.openjdk.java.net/jdk/pull/155/files - new: https://git.openjdk.java.net/jdk/pull/155/files/6541a83f..775b3630 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=155=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=155=00-01 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/155.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/155/head:pull/155 PR: https://git.openjdk.java.net/jdk/pull/155
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v3]
On Tue, 15 Sep 2020 13:40:29 GMT, Ian Graves wrote: >> Related to [JDK-8252730 jlink does not create reproducible builds on >> different >> servers](https://bugs.openjdk.java.net/browse/JDK-8252730). Introduces >> ordering based on `Archive` module names to >> ensure stable files (and file signatures) across generated JDK images by >> jlink. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Comparator cleanup The update using flatMap looks good. I think we need to explore adding more tests if possible. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/156
Integrated: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 07:47:54 GMT, Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, mostly > in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized This pull request has now been integrated. Changeset: b5620a36 Author:Christoph Langer URL: https://git.openjdk.java.net/jdk/commit/b5620a36 Stats: 21 lines in 4 files changed: 7 ins; 3 del; 11 mod 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming Reviewed-by: dfuchs, aefimov, alanb, vtewari - PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v3]
> Related to [JDK-8252730 jlink does not create reproducible builds on different > servers](https://bugs.openjdk.java.net/browse/JDK-8252730). Introduces > ordering based on `Archive` module names to > ensure stable files (and file signatures) across generated JDK images by > jlink. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Comparator cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/156/files - new: https://git.openjdk.java.net/jdk/pull/156/files/ef3eacec..b89d2c4d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=156=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=156=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/156.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/156/head:pull/156 PR: https://git.openjdk.java.net/jdk/pull/156
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v2]
On Tue, 15 Sep 2020 09:32:33 GMT, Daniel Fuchs wrote: >> Ian Graves has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Merging streams > > src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java > line 41: > >> 39: import java.util.Objects; >> 40: import java.util.Set; >> 41: import java.util.Comparator; > > nit: `Comparator` might no longer be needed? I don't see where it's used. Yep. Looks like it's a leftover. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/156
Re: RFR: 8216497: javadoc should auto-link to platform classes
On Tue, 15 Sep 2020 12:56:13 GMT, Erik Joelsson wrote: >> This pull request is identical with the RFR previously sent for the >> Mercurial repository: >> >> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html >> >> I'm copy-pasting the comments from the original RFR below. >> >> Most of the new code is added to the Extern class where it fits in quite >> nicely and can use the existing supporting >> code for setting up external links. >> The default behaviour is to generate links to docs.oracle.com for released >> versions and download.java.net for >> prereleases. Platform documentation URLs can be configured using the new >> --link-platform-properties option to >> provide a properties file with URLs pointing to to alternative locations. >> See the CSR linked above for more details on >> the new options. The feature can be disabled using the --no-platform-link >> option, generating the same output as >> previously. One problem I had to solve was how to handle the transition >> from prerelease versions to final releases, >> since the location of the documentation changes in this transition. For >> obvious reasons we don’t want to make that >> switch via code change at a time shortly before release. The way it is done >> is that we determine if the current >> javadoc instance is a prerelease version as indicated by the Version >> returned by BaseConfiguration#getDocletVersion(), >> and then check whether the release/source version of the current javadoc >> execution uses the same (latest) version. This >> means that that only the latest version will ever generate prerelease links >> (e.g. running current javadoc 16 with >> source version 15 will generate links to the final release documentation) >> but I think this is acceptable. Another >> issue I spent some time getting right was tests. New releases require a new >> element-list resource*), so tests have to >> pick up new releases. On the other hand, we don’t want hundreds of tests to >> fail when a new release is added, ideally >> there should be one test with a clear message. Because of this, when a >> release is encountered for which no >> element-list is available a warning is generated instead of an error, and >> the documentation is generated without >> platform links as if running with --no-platform-link option. This allows >> most existing tests to pass and just the new >> test to fail with a relatively clear message of what is wrong. >> *) I also thought about generating the element-list for the current release >> at build time. It’s quite involved, and we >> still need to maintain element-lists for older versions, so I’m not sure >> it’s worth it. >> >> For existing tests that check output affected by the new option I added the >> --no-platform-link option to disable the >> feature. Otherwise we’d have to update those tests with each new release (or >> else freeze the tests to use some static >> release or source version, which we don’t want either). I updated the CSR >> to the new code. It also needs to be >> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181 >> >> Thanks, >> Hannes > > Build changes look good. Converted to draft as @lahodaj has offered to enhance the feature as described in the comments above. - PR: https://git.openjdk.java.net/jdk/pull/171
Re: RFR: 8216497: javadoc should auto-link to platform classes
On Tue, 15 Sep 2020 09:10:54 GMT, Hannes Wallnöfer wrote: > This pull request is identical with the RFR previously sent for the Mercurial > repository: > > https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html > > I'm copy-pasting the comments from the original RFR below. > > Most of the new code is added to the Extern class where it fits in quite > nicely and can use the existing supporting > code for setting up external links. > The default behaviour is to generate links to docs.oracle.com for released > versions and download.java.net for > prereleases. Platform documentation URLs can be configured using the new > --link-platform-properties option to > provide a properties file with URLs pointing to to alternative locations. See > the CSR linked above for more details on > the new options. The feature can be disabled using the --no-platform-link > option, generating the same output as > previously. One problem I had to solve was how to handle the transition > from prerelease versions to final releases, > since the location of the documentation changes in this transition. For > obvious reasons we don’t want to make that > switch via code change at a time shortly before release. The way it is done > is that we determine if the current > javadoc instance is a prerelease version as indicated by the Version returned > by BaseConfiguration#getDocletVersion(), > and then check whether the release/source version of the current javadoc > execution uses the same (latest) version. This > means that that only the latest version will ever generate prerelease links > (e.g. running current javadoc 16 with > source version 15 will generate links to the final release documentation) but > I think this is acceptable. Another > issue I spent some time getting right was tests. New releases require a new > element-list resource*), so tests have to > pick up new releases. On the other hand, we don’t want hundreds of tests to > fail when a new release is added, ideally > there should be one test with a clear message. Because of this, when a > release is encountered for which no > element-list is available a warning is generated instead of an error, and the > documentation is generated without > platform links as if running with --no-platform-link option. This allows most > existing tests to pass and just the new > test to fail with a relatively clear message of what is wrong. > *) I also thought about generating the element-list for the current release > at build time. It’s quite involved, and we > still need to maintain element-lists for older versions, so I’m not sure > it’s worth it. > > For existing tests that check output affected by the new option I added the > --no-platform-link option to disable the > feature. Otherwise we’d have to update those tests with each new release (or > else freeze the tests to use some static > release or source version, which we don’t want either). I updated the CSR to > the new code. It also needs to be > reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181 > > Thanks, > Hannes Build changes look good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/171
Re: RFR: 8216497: javadoc should auto-link to platform classes
On Tue, 15 Sep 2020 11:30:09 GMT, Jan Lahoda wrote:
>> This pull request is identical with the RFR previously sent for the
>> Mercurial repository:
>>
>> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
>>
>> I'm copy-pasting the comments from the original RFR below.
>>
>> Most of the new code is added to the Extern class where it fits in quite
>> nicely and can use the existing supporting
>> code for setting up external links.
>> The default behaviour is to generate links to docs.oracle.com for released
>> versions and download.java.net for
>> prereleases. Platform documentation URLs can be configured using the new
>> --link-platform-properties option to
>> provide a properties file with URLs pointing to to alternative locations.
>> See the CSR linked above for more details on
>> the new options. The feature can be disabled using the --no-platform-link
>> option, generating the same output as
>> previously. One problem I had to solve was how to handle the transition
>> from prerelease versions to final releases,
>> since the location of the documentation changes in this transition. For
>> obvious reasons we don’t want to make that
>> switch via code change at a time shortly before release. The way it is done
>> is that we determine if the current
>> javadoc instance is a prerelease version as indicated by the Version
>> returned by BaseConfiguration#getDocletVersion(),
>> and then check whether the release/source version of the current javadoc
>> execution uses the same (latest) version. This
>> means that that only the latest version will ever generate prerelease links
>> (e.g. running current javadoc 16 with
>> source version 15 will generate links to the final release documentation)
>> but I think this is acceptable. Another
>> issue I spent some time getting right was tests. New releases require a new
>> element-list resource*), so tests have to
>> pick up new releases. On the other hand, we don’t want hundreds of tests to
>> fail when a new release is added, ideally
>> there should be one test with a clear message. Because of this, when a
>> release is encountered for which no
>> element-list is available a warning is generated instead of an error, and
>> the documentation is generated without
>> platform links as if running with --no-platform-link option. This allows
>> most existing tests to pass and just the new
>> test to fail with a relatively clear message of what is wrong.
>> *) I also thought about generating the element-list for the current release
>> at build time. It’s quite involved, and we
>> still need to maintain element-lists for older versions, so I’m not sure
>> it’s worth it.
>>
>> For existing tests that check output affected by the new option I added the
>> --no-platform-link option to disable the
>> feature. Otherwise we’d have to update those tests with each new release (or
>> else freeze the tests to use some static
>> release or source version, which we don’t want either). I updated the CSR
>> to the new code. It also needs to be
>> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
>>
>> Thanks,
>> Hannes
>
> I think it would be awesome if we could generate (most of) the
> {element,package}-list-VERSION.txt from the ct.sym
> historical data at build time. This would (hopefully) help with long-term
> maintenance. I've started to sketch that
> here:
> https://github.com/lahodaj/jdk/commit/36c1510587a4b050b148eda87ae7a7a89aff9690
> Some comments on the attempt:
> -in this PR, there is package-list-9.txt - should it be element-list-9.txt,
> and should it contain module names (dtto
> element-list-10.txt)?
> -we may (for historical reasons) want to keep the lists for 7, 8, 9 and 10
> (as the historical data in ct.sym do not
> exactly match what is in the package/element lists). It would be better to
> generate everything, but having a fixed list
> for some of the past versions would be better than having to create a new
> list for each new release.
> -the patch does not yet generate the list for the current release, but that
> should be doable.
Thanks for the suggestions and help, Jan!
> I think it would be awesome if we could generate (most of) the
> {element,package}-list-VERSION.txt from the ct.sym
> historical data at build time. This would (hopefully) help with long-term
> maintenance. I've started to sketch that
> here:
> [lahodaj@36c1510](https://github.com/lahodaj/jdk/commit/36c1510587a4b050b148eda87ae7a7a89aff9690)
I agree files should be generated dynamically. I knew about the sym files but
wasn't sure how to go about it. Thanks a
lot for stepping in and helping out, it's very much appreciated!
> Some comments on the attempt:
> -in this PR, there is package-list-9.txt - should it be element-list-9.txt,
> and should it contain module names (dtto
> element-list-10.txt)?
Javadoc in 9 still uses the old package-centric layout (package-list and no
module names in URL paths). It
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v4]
> Can I please get a review and a sponsor for this patch which fixes the issue > reported in > https://bugs.openjdk.java.net/browse/JDK-8244706? > The commit here sets the `OS` header flag to `255` (which represents > `unknown`) as noted in [1]. A new test has been > included in this commit to verify the change. Furthermore, this doesn't > impact the `java.util.zip.GZIPInputStream` > since it ignores [2] this header value while reading the headers from the > stream. [1] > https://tools.ietf.org/html/rfc1952#page-7 [2] > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173 Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Remove unintended file commit - Changes: - all: https://git.openjdk.java.net/jdk/pull/130/files - new: https://git.openjdk.java.net/jdk/pull/130/files/7679b119..a510dc77 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=130=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=130=02-03 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/130.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/130/head:pull/130 PR: https://git.openjdk.java.net/jdk/pull/130
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v4]
On Sun, 13 Sep 2020 02:22:54 GMT, Jaikiran Pai wrote: >> Hi Jaikiran, >> >> The change seems fine an inline with the RFC. I can sponsor this once we >> have another review. >> >> I have run the JCK tests for Zip/Gzip/Jar and Mach5 JDK tier1, tier2 and >> tier3 > > Thank you Lance for the review and running the tests. I'll wait for another > review before initiating the integrate > command. I received some useful suggestions, offline, from Lance and Brent on this change and have updated this PR to include those suggestions. Please ignore the intermediate commits though (I accidentally added an unwanted file to this PR, which I have then removed in a subsequent commit). This is ready for a review now. - PR: https://git.openjdk.java.net/jdk/pull/130
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v3]
> Can I please get a review and a sponsor for this patch which fixes the issue > reported in > https://bugs.openjdk.java.net/browse/JDK-8244706? > The commit here sets the `OS` header flag to `255` (which represents > `unknown`) as noted in [1]. A new test has been > included in this commit to verify the change. Furthermore, this doesn't > impact the `java.util.zip.GZIPInputStream` > since it ignores [2] this header value while reading the headers from the > stream. [1] > https://tools.ietf.org/html/rfc1952#page-7 [2] > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173 Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Improve the comment for the OS_UNKNOWN field - Changes: - all: https://git.openjdk.java.net/jdk/pull/130/files - new: https://git.openjdk.java.net/jdk/pull/130/files/159684de..7679b119 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=130=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=130=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/130.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/130/head:pull/130 PR: https://git.openjdk.java.net/jdk/pull/130
Re: RFR: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) [v2]
> Can I please get a review and a sponsor for this patch which fixes the issue > reported in > https://bugs.openjdk.java.net/browse/JDK-8244706? > The commit here sets the `OS` header flag to `255` (which represents > `unknown`) as noted in [1]. A new test has been > included in this commit to verify the change. Furthermore, this doesn't > impact the `java.util.zip.GZIPInputStream` > since it ignores [2] this header value while reading the headers from the > stream. [1] > https://tools.ietf.org/html/rfc1952#page-7 [2] > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173 Jaikiran Pai 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: 8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) Reviewed-By: - Changes: - all: https://git.openjdk.java.net/jdk/pull/130/files - new: https://git.openjdk.java.net/jdk/pull/130/files/afad1261..159684de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=130=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=130=00-01 Stats: 5 lines in 2 files changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/130.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/130/head:pull/130 PR: https://git.openjdk.java.net/jdk/pull/130
Re: RFR: 8202473: A type variable with multiple bounds does not correctly place type annotation
On Tue, 15 Sep 2020 08:10:00 GMT, Joel Borggrén-Franck wrote: >> This patch assures that an annotation on a type bound is placed on the >> correct bound index. > > Pre-git review thread here: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067049.html @raphw test is missing imports and doesn't compile, other than that looks good. - PR: https://git.openjdk.java.net/jdk/pull/155
Re: RFR: 8216497: javadoc should auto-link to platform classes
On Tue, 15 Sep 2020 09:10:54 GMT, Hannes Wallnöfer wrote:
> This pull request is identical with the RFR previously sent for the Mercurial
> repository:
>
> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
>
> I'm copy-pasting the comments from the original RFR below.
>
> Most of the new code is added to the Extern class where it fits in quite
> nicely and can use the existing supporting
> code for setting up external links.
> The default behaviour is to generate links to docs.oracle.com for released
> versions and download.java.net for
> prereleases. Platform documentation URLs can be configured using the new
> --link-platform-properties option to
> provide a properties file with URLs pointing to to alternative locations. See
> the CSR linked above for more details on
> the new options. The feature can be disabled using the --no-platform-link
> option, generating the same output as
> previously. One problem I had to solve was how to handle the transition
> from prerelease versions to final releases,
> since the location of the documentation changes in this transition. For
> obvious reasons we don’t want to make that
> switch via code change at a time shortly before release. The way it is done
> is that we determine if the current
> javadoc instance is a prerelease version as indicated by the Version returned
> by BaseConfiguration#getDocletVersion(),
> and then check whether the release/source version of the current javadoc
> execution uses the same (latest) version. This
> means that that only the latest version will ever generate prerelease links
> (e.g. running current javadoc 16 with
> source version 15 will generate links to the final release documentation) but
> I think this is acceptable. Another
> issue I spent some time getting right was tests. New releases require a new
> element-list resource*), so tests have to
> pick up new releases. On the other hand, we don’t want hundreds of tests to
> fail when a new release is added, ideally
> there should be one test with a clear message. Because of this, when a
> release is encountered for which no
> element-list is available a warning is generated instead of an error, and the
> documentation is generated without
> platform links as if running with --no-platform-link option. This allows most
> existing tests to pass and just the new
> test to fail with a relatively clear message of what is wrong.
> *) I also thought about generating the element-list for the current release
> at build time. It’s quite involved, and we
> still need to maintain element-lists for older versions, so I’m not sure
> it’s worth it.
>
> For existing tests that check output affected by the new option I added the
> --no-platform-link option to disable the
> feature. Otherwise we’d have to update those tests with each new release (or
> else freeze the tests to use some static
> release or source version, which we don’t want either). I updated the CSR to
> the new code. It also needs to be
> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
>
> Thanks,
> Hannes
I think it would be awesome if we could generate (most of) the
{element,package}-list-VERSION.txt from the ct.sym
historical data at build time. This would (hopefully) help with long-term
maintenance. I've started to sketch that
here:
https://github.com/lahodaj/jdk/commit/36c1510587a4b050b148eda87ae7a7a89aff9690
Some comments on the attempt:
-in this PR, there is package-list-9.txt - should it be element-list-9.txt, and
should it contain module names (dtto
element-list-10.txt)?
-we may (for historical reasons) want to keep the lists for 7, 8, 9 and 10 (as
the historical data in ct.sym do not
exactly match what is in the package/element lists). It would be better to
generate everything, but having a fixed list
for some of the past versions would be better than having to create a new list
for each new release.
-the patch does not yet generate the list for the current release, but that
should be doable.
-
PR: https://git.openjdk.java.net/jdk/pull/171
RFR: 8250859: Address reliance on default constructors in the Accessibility APIs
This issue relates to JDK-8250639 '☂ Address reliance on default constructors in the java.desktop module'. The following classes have had an explicit no-arg constructor added, with a protected access modifier and accompanying API description: - Default ctor on com.sun.java.accessibility.util.SwingEventMonitor - Default ctor on javax.accessibility.AccessibleContext - Default ctor on javax.accessibility.AccessibleHyperlink The following classes have had an explicit no-arg constructor added, with a public access modifier and accompanying API description: - Default ctor on javax.accessibility.AccessibleResourceBundle - Default ctor on com.sun.java.accessibility.util.AWTEventMonitor - Default ctor on com.sun.java.accessibility.util.AccessibilityEventMonitor - Default ctor on com.sun.java.accessibility.util.AccessibilityListenerList - Default ctor on com.sun.java.accessibility.util.EventID specdiff: http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8250859/webrevs/webrev.00/specdiff/overview-summary.html bug: https://bugs.openjdk.java.net/browse/JDK-8250859 - Commit messages: - 8250859: Address reliance on default constructors in the Accessibility APIs Changes: https://git.openjdk.java.net/jdk/pull/174/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=174=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8250859 Stats: 42 lines in 7 files changed: 35 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/174.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/174/head:pull/174 PR: https://git.openjdk.java.net/jdk/pull/174
Re: RFR: 8250859: Address reliance on default constructors in the Accessibility APIs
On Tue, 15 Sep 2020 10:04:49 GMT, Conor Cleary wrote: > This issue relates to JDK-8250639 '☂ Address reliance on default constructors > in the java.desktop module'. The > following classes have had an explicit no-arg constructor added, with a > protected access modifier and accompanying API > description: > - Default ctor on com.sun.java.accessibility.util.SwingEventMonitor > - Default ctor on javax.accessibility.AccessibleContext > - Default ctor on javax.accessibility.AccessibleHyperlink > > The following classes have had an explicit no-arg constructor added, with a > public access modifier and accompanying API > description: > - Default ctor on javax.accessibility.AccessibleResourceBundle > - Default ctor on com.sun.java.accessibility.util.AWTEventMonitor > - Default ctor on com.sun.java.accessibility.util.AccessibilityEventMonitor > - Default ctor on com.sun.java.accessibility.util.AccessibilityListenerList > - Default ctor on com.sun.java.accessibility.util.EventID > > specdiff: > http://cr.openjdk.java.net/~ccleary/issues/webrevs-store/8250859/webrevs/webrev.00/specdiff/overview-summary.html > bug: > https://bugs.openjdk.java.net/browse/JDK-8250859 Changes require approval of CSR, can't request that requirement myself as I am not a reviewer - PR: https://git.openjdk.java.net/jdk/pull/174
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 07:47:54 GMT, Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, mostly > in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized Looks okay, no semantic changes. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 09:39:29 GMT, Daniel Fuchs wrote: >> There are some little flaws in LdapDNSProvider and auxilliary classes, >> mostly in Javadoc. >> >> In detail: >> src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: >> Unnecessary import >> src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: >> typo >> src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: >> Whitespace >> src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: >> Spelling of "ldap" -> should be >> capitalized > > Hi Christoph, > > The changes look good to me. > > best regards, > > -- daniel Hi Christoph, Looks good to me. Kind Regards, Aleksei - PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 07:47:54 GMT, Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, mostly > in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
On Tue, 15 Sep 2020 07:47:54 GMT, Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, mostly > in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized Hi Christoph, The changes look good to me. best regards, -- daniel - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/168
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v2]
On Mon, 14 Sep 2020 22:57:44 GMT, Ian Graves wrote: >> Related to [JDK-8252730 jlink does not create reproducible builds on >> different >> servers](https://bugs.openjdk.java.net/browse/JDK-8252730). Introduces >> ordering based on `Archive` module names to >> ensure stable files (and file signatures) across generated JDK images by >> jlink. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Merging streams src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 41: > 39: import java.util.Objects; > 40: import java.util.Set; > 41: import java.util.Comparator; nit: `Comparator` might no longer be needed? I don't see where it's used. - PR: https://git.openjdk.java.net/jdk/pull/156
RFR: 8216497: javadoc should auto-link to platform classes
This pull request is identical with the RFR previously sent for the Mercurial repository: https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html I'm copy-pasting the comments from the original RFR below. Most of the new code is added to the Extern class where it fits in quite nicely and can use the existing supporting code for setting up external links. The default behaviour is to generate links to docs.oracle.com for released versions and download.java.net for prereleases. Platform documentation URLs can be configured using the new --link-platform-properties option to provide a properties file with URLs pointing to to alternative locations. See the CSR linked above for more details on the new options. The feature can be disabled using the --no-platform-link option, generating the same output as previously. One problem I had to solve was how to handle the transition from prerelease versions to final releases, since the location of the documentation changes in this transition. For obvious reasons we don’t want to make that switch via code change at a time shortly before release. The way it is done is that we determine if the current javadoc instance is a prerelease version as indicated by the Version returned by BaseConfiguration#getDocletVersion(), and then check whether the release/source version of the current javadoc execution uses the same (latest) version. This means that that only the latest version will ever generate prerelease links (e.g. running current javadoc 16 with source version 15 will generate links to the final release documentation) but I think this is acceptable. Another issue I spent some time getting right was tests. New releases require a new element-list resource*), so tests have to pick up new releases. On the other hand, we don’t want hundreds of tests to fail when a new release is added, ideally there should be one test with a clear message. Because of this, when a release is encountered for which no element-list is available a warning is generated instead of an error, and the documentation is generated without platform links as if running with --no-platform-link option. This allows most existing tests to pass and just the new test to fail with a relatively clear message of what is wrong. *) I also thought about generating the element-list for the current release at build time. It’s quite involved, and we still need to maintain element-lists for older versions, so I’m not sure it’s worth it. For existing tests that check output affected by the new option I added the --no-platform-link option to disable the feature. Otherwise we’d have to update those tests with each new release (or else freeze the tests to use some static release or source version, which we don’t want either). I updated the CSR to the new code. It also needs to be reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181 Thanks, Hannes - Commit messages: - 8216497: javadoc should auto-link to platform classes Changes: https://git.openjdk.java.net/jdk/pull/171/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=171=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8216497 Stats: 3230 lines in 53 files changed: 3220 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/171.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/171/head:pull/171 PR: https://git.openjdk.java.net/jdk/pull/171
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
Hi Christoph, Changes look ok to me. On Tue, Sep 15, 2020 at 1:26 PM Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, > mostly in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized > > - > > Commit messages: > - JDK-8253155 > > Changes: https://git.openjdk.java.net/jdk/pull/168/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=168=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8253155 > Stats: 21 lines in 4 files changed: 3 ins; 7 del; 11 mod > Patch: https://git.openjdk.java.net/jdk/pull/168.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/168/head:pull/168 > > PR: https://git.openjdk.java.net/jdk/pull/168 > -- Thanks, Vyom
Re: RFR: 8202473: A type variable with multiple bounds does not correctly place type annotation
On Mon, 14 Sep 2020 19:49:08 GMT, Rafael Winterhalter wrote: > This patch assures that an annotation on a type bound is placed on the > correct bound index. Pre-git review thread here: https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067049.html - PR: https://git.openjdk.java.net/jdk/pull/155
Re: RFR: 8252730: jlink does not create reproducible builds on different servers [v2]
On Mon, 14 Sep 2020 22:54:34 GMT, Ian Graves wrote: >> Looks good to me. > > Updated to merge streams in line with @stuart-marks suggestion. There are several existing jlink tests that check that generated lib/modules is reproducible. Can we do more in these tests or add a new test that would improve the chances of finding issues, e.g. using jlink to create a JDK that includes the jlink tool and the packaged modules, copy it to two locations and run the jlink in both run-time images to see if they produce the same bits. - PR: https://git.openjdk.java.net/jdk/pull/156
RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
There are some little flaws in LdapDNSProvider and auxilliary classes, mostly in Javadoc. In detail: src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: Unnecessary import src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: typo src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: Whitespace src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: Spelling of "ldap" -> should be capitalized - Commit messages: - JDK-8253155 Changes: https://git.openjdk.java.net/jdk/pull/168/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=168=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253155 Stats: 21 lines in 4 files changed: 3 ins; 7 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/168.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/168/head:pull/168 PR: https://git.openjdk.java.net/jdk/pull/168
