Re: JDK 12 RFR of JDK-8213911: Use example.com in java.net and other examples
Looks good to me. -Pavel
RFR [15] 8241014: Miscellaneous typos in documentation comments
Hello, Please review the change for https://bugs.openjdk.java.net/browse/JDK-8241014: http://cr.openjdk.java.net/~prappo/8241014/webrev.00/ This is a documentation cleanup. There are no code changes involved, and the changes in documentation are mostly trivial. The following packages are affected: java.lang, java.nio.file, java.nio.file.attribute, java.security, java.time.chrono, java.time.temporal, java.util, java.util.regex, java.util.stream, javax.crypto, javax.security.cert, javax.tools That said, there are two changes that I'd prefer to be carefully reviewed by the experts in the corresponding areas. The first one is for a suspected typo in the javax.crypto.CryptoPolicyParser class, "AlgrithomParameterSpec". It is not unheard-of for typos to be kept and supported for the sake of backward compatibility. Sadly, we have a number of those in OpenJDK. Even though I performed reasonable checks, the proposed fix should better be verified by the security folk. The second one is for the doc comment for the java.util.stream.Stream.collect method. @apiNote The following will accumulate strings into an ArrayList: List asList = stringStream.collect(Collectors.toList()); Given that the spec for Collectors.toList() clearly says that ...There are no guarantees on the type, mutability, serializability, or thread-safety of the List returned;... I'd assume that @apiNote should be fixed as proposed. -Pavel P.S. Apologies for spamming multiple mailing lists.
Re: RFR [15] 8241014: Miscellaneous typos in documentation comments
Hi Ivan, Your changes look good to me. Thanks for doing this. I will merge your changes into that patch of mine. 1. One thing that your change highlighted is that use of FQN here: public int read(java.nio.CharBuffer target) throws IOException It's been there for ages, but I believe can now safely go away after $ hg log -v -r 49262 changeset: 49262:1b3ee04e3e54 user:rriggs date:Mon Mar 19 09:58:41 2018 -0400 files: src/java.base/share/classes/java/io/Reader.java src/java.base/share/classes/java/io/Writer.java test/jdk/java/io/Reader/NullReader.java test/jdk/java/io/Writer/NullWriter.java description: 8196298: Add null Reader and Writer Reviewed-by: bpb, forax, smarks, alanb, rriggs Contributed-by: patr...@reini.net as this latter change brought import java.nio.CharBuffer; Unless people want to keep that FQN for some other reasons. For example, to emphasize that CharBuffer does not belong to java.io, or so it does not get confused with unrelated concepts like BufferedReader.defaultCharBufferSize, etc. 2. Typos aside, this MethodType.ConcurrentWeakInternSet.WeakEntry#equals has peculiar semantics! Paul Sandoz might be a good fit for reviewing changes in both java.util.stream (mine) and java.util.Arrays (yours). -Pavel > On 14 Mar 2020, at 02:03, Ivan Gerasimov wrote: > > Hi Pavel! > > Can this please be combined with my collection of typos? > > http://cr.openjdk.java.net/~igerasim/XXX-typos/00/webrev/ > > Just to save cycles on reviewing :) > > With kind regards, > > Ivan > > > On 3/13/20 8:42 AM, Pavel Rappo wrote: >> Hello, >> >> Please review the change for >> https://bugs.openjdk.java.net/browse/JDK-8241014: >> >> http://cr.openjdk.java.net/~prappo/8241014/webrev.00/ >> >> This is a documentation cleanup. There are no code changes involved, >> and the changes in documentation are mostly trivial. >> >> The following packages are affected: >> >> java.lang, >> java.nio.file, >> java.nio.file.attribute, >> java.security, >> java.time.chrono, >> java.time.temporal, >> java.util, >> java.util.regex, >> java.util.stream, >> javax.crypto, >> javax.security.cert, >> javax.tools >> >> That said, there are two changes that I'd prefer to be carefully reviewed by >> the experts in the corresponding areas. >> >> The first one is for a suspected typo in the javax.crypto.CryptoPolicyParser >> class, "AlgrithomParameterSpec". It is not unheard-of for typos to be kept >> and >> supported for the sake of backward compatibility. Sadly, we have a number of >> those in OpenJDK. Even though I performed reasonable checks, the proposed fix >> should better be verified by the security folk. >> >> The second one is for the doc comment for the >> java.util.stream.Stream.collect method. >> >> @apiNote >> The following will accumulate strings into an ArrayList: >> >> List asList = stringStream.collect(Collectors.toList()); >> >> Given that the spec for Collectors.toList() clearly says that >> >> ...There are no guarantees on the type, mutability, serializability, or >> thread-safety of the List returned;... >> >> I'd assume that @apiNote should be fixed as proposed. >> >> -Pavel >> >> P.S. Apologies for spamming multiple mailing lists. >> > -- > With kind regards, > Ivan Gerasimov >
Re: RFR [15] 8241014: Miscellaneous typos in documentation comments
Thanks Ivan, I merged your changes with mine and pushed the resulting changeset.
> On 20 Mar 2020, at 20:11, Ivan Gerasimov wrote:
>
> Thank you Paul!
>
> grep found a few more occurrences of 'equals to' across java.base, so I
> fixed them as well.
>
> Here's the updated webrev:
>
> http://cr.openjdk.java.net/~igerasim/XXX-typos/01/webrev/
>
>
> Pavel, I checked your portion of correction, everything looks good to me!
>
> One minor nit: In src/java.base/share/classes/java/util/StringJoiner.java
> can you please wrap the modified line, so it won't be that long?
>
>
> With kind regards,
>
> Ivan
>
>
> On 3/20/20 10:16 AM, Paul Sandoz wrote:
>> --- a/src/java.base/share/classes/java/lang/invoke/MethodType.java
>> +++ b/src/java.base/share/classes/java/lang/invoke/MethodType.java
>> @@ -1379,12 +1379,12 @@
>>/**
>> * This implementation returns {@code true} if {@code obj} is
>> another
>> - * {@code WeakEntry} whose referent is equals to this referent,
>> or
>> - * if {@code obj} is equals to the referent of this. This allows
>> + * {@code WeakEntry} whose referent equals to this referent, or
>> + * if {@code obj} equals to the referent of this. This allows
>> * lookups to be made without wrapping in a {@code WeakEntry}.
>> *
>> * @param obj the object to compare
>> - * @return true if {@code obj} is equals to this or the
>> referent of this
>> + * @return true if {@code obj} equals to this or the referent
>> of this
>> * @see MethodType#equals(Object)
>> * @see Object#equals(Object)
>> Use either:
>>
>> whose referent is equal to this referent,
>>
>> or
>>
>> whose referent equals this referent,
>>
>> The former is easier just delete the ’s’.
>>
>> Other bits look good.
>>
>> Paul.
>>
>>> On Mar 13, 2020, at 7:03 PM, Ivan Gerasimov
>>> wrote:
>>>
>>> Hi Pavel!
>>>
>>> Can this please be combined with my collection of typos?
>>>
>>> http://cr.openjdk.java.net/~igerasim/XXX-typos/00/webrev/
>>>
>>> Just to save cycles on reviewing :)
>>>
>>> With kind regards,
>>>
>>> Ivan
>>>
>>>
>>> On 3/13/20 8:42 AM, Pavel Rappo wrote:
>>>> Hello,
>>>>
>>>> Please review the change for
>>>> https://bugs.openjdk.java.net/browse/JDK-8241014:
>>>>
>>>> http://cr.openjdk.java.net/~prappo/8241014/webrev.00/
>>>>
>>>> This is a documentation cleanup. There are no code changes involved,
>>>> and the changes in documentation are mostly trivial.
>>>>
>>>> The following packages are affected:
>>>>
>>>> java.lang,
>>>> java.nio.file,
>>>> java.nio.file.attribute,
>>>> java.security,
>>>> java.time.chrono,
>>>> java.time.temporal,
>>>> java.util,
>>>> java.util.regex,
>>>> java.util.stream,
>>>> javax.crypto,
>>>> javax.security.cert,
>>>> javax.tools
>>>>
>>>> That said, there are two changes that I'd prefer to be carefully reviewed
>>>> by
>>>> the experts in the corresponding areas.
>>>>
>>>> The first one is for a suspected typo in the
>>>> javax.crypto.CryptoPolicyParser
>>>> class, "AlgrithomParameterSpec". It is not unheard-of for typos to be kept
>>>> and
>>>> supported for the sake of backward compatibility. Sadly, we have a number
>>>> of
>>>> those in OpenJDK. Even though I performed reasonable checks, the proposed
>>>> fix
>>>> should better be verified by the security folk.
>>>>
>>>> The second one is for the doc comment for the
>>>> java.util.stream.Stream.collect method.
>>>>
>>>> @apiNote
>>>> The following will accumulate strings into an ArrayList:
>>>>
>>>> List asList = stringStream.collect(Collectors.toList());
>>>>
>>>> Given that the spec for Collectors.toList() clearly says that
>>>>
>>>> ...There are no guarantees on the type, mutability, serializability, or
>>>> thread-safety of the List returned;...
>>>>
>>>> I'd assume that @apiNote should be fixed as proposed.
>>>>
>>>> -Pavel
>>>>
>>>> P.S. Apologies for spamming multiple mailing lists.
>>>>
>>> --
>>> With kind regards,
>>> Ivan Gerasimov
>>>
> --
> With kind regards,
> Ivan Gerasimov
>
Re: Need sponsor to fix Javadoc warnings
Vipin, here you go: https://bugs.openjdk.java.net/browse/JDK-8242366 http://cr.openjdk.java.net/~prappo/8242366/webrev.00/ I took the liberty of additionally fixing a couple of parameters' names, a typo, and `@exception` tags for checked exceptions that were neither thrown nor imported. The bulk of the change is in Security. Some changes are in Networking. The appropriate mailing lists are in CC for this email. We should wait for their feedback. Changes in core area look good to me and I'd be surprised if there are any problems with the remaining portion of the changeset. -Pavel > On 7 Apr 2020, at 19:50, Vipin Sharma wrote: > > Hi Pavel, > >> On Apr 7, 2020, at 11:11 PM, Pavel Rappo wrote: >> >> I assume you have signed the OCA [1]. If not and you want to continue, >> please do it. If you've already done so, which is probably the case [2], >> please attach your patch as text to this thread with the next email. Do not >> use zip or the like. I will take it from there and sponsor that for you. > Yes I have signed OCA. >> >> -Pavel >> >> [1] https://www.oracle.com/technetwork/community/oca-486395.html >> [2] changeset: 58344:65f30e209890 >> user:clanger >> date:Wed Mar 11 13:50:13 2020 +0100 >> files: test/jdk/java/lang/Boolean/GetBoolean.java >> test/jdk/java/lang/Boolean/MakeBooleanComparable.java >> test/jdk/java/lang/Boolean/ParseBoolean.java >> description: >> 8240524: Remove explicit type argument in test >> jdk/java/lang/Boolean/MakeBooleanComparable.java >> Reviewed-by: clanger, vtewari >> Contributed-by: vipinsharma85 at gmail.com >> > Yes this is my first contribution. > > Patch text: > > --- old/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java > 2020-04-06 00:19:10.546117441 +0530 > +++ new/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java > 2020-04-06 00:19:10.130115855 +0530 > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights > reserved. > + * Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights > reserved. > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > * > * This code is free software; you can redistribute it and/or modify it > @@ -202,7 +202,7 @@ > /** > * Sets the padding mechanism of this cipher. > * > - * @param padding the padding mechanism > + * @param paddingScheme the padding mechanism > * > * @exception NoSuchPaddingException if the requested padding mechanism > * does not exist > --- > old/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java > 2020-04-06 00:19:11.526121179 +0530 > +++ > new/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java > 2020-04-06 00:19:11.118119622 +0530 > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2004, 2017, Oracle and/or its affiliates. All rights > reserved. > + * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights > reserved. > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > * > * This code is free software; you can redistribute it and/or modify it > @@ -313,10 +313,10 @@ > * current Cipher.engineInit(...) implementation, > * IllegalStateException will always be thrown upon invocation. > * > - * @param in the input buffer > - * @param inOffset the offset in in where the input > + * @param input the input buffer > + * @param inputOffset the offset in in where the input > * starts > - * @param inLen the input length. > + * @param inputLen the input length. > * > * @return n/a. > * > --- > old/src/java.base/share/classes/com/sun/crypto/provider/BlowfishCrypt.java > 2020-04-06 00:19:12.462124749 +0530 > +++ > new/src/java.base/share/classes/com/sun/crypto/provider/BlowfishCrypt.java > 2020-04-06 00:19:12.054123193 +0530 > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 1998, 2007, Oracle and/or its affiliates. All rights > reserved. > + * Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights > reserved. > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > * > * This code is free software; you can redistribute it and/or modify it > @@ -130,7 +130,6 @@ > * > * @param plain the buffer with the input data to be encrypted > * @param plainOffset the offset in plain > - * @param plainLen the length of the input data > * @param cipher the buffer for the result > * @param cipherOffset the offset in cipher > */ > @@ -154,7 +153,6
Re: Need sponsor to fix Javadoc warnings
Why assume something that sophisticated where it can be adequately explained by a simpler thing? :) I bet it was an IDE inspection. -Pavel > On 8 Apr 2020, at 14:14, Alan Bateman wrote: > > On 08/04/2020 14:07, Daniel Fuchs wrote: >> Hi Pavel, >> >> On 08/04/2020 13:56, David Holmes wrote: >>> and `@exception` tags for checked exceptions that were neither thrown >>> nor imported >> >> Hopefully that's only on internal classes. > From a quick scan, the changes are to internal classes and a few non-public > elements of public API classes. So I don't think anything should impact the > javadoc. I'm guessing this patch is motivated by something creating that is > running the javadoc tool with different options to what the "docs" target > uses. > > -Alan
Re: Need sponsor to fix Javadoc warnings
Hey David,
Where exactly? In the files affected by this changeset? If so, then we will
introduce inconsistency. Otherwise it's a huge change. From what I can see there
are some 250 occurrences of `@exception` in
src/java.base/share/classes/com/sun/{crypto, security}
and some 7,300 in src.
Personally, out of all tag renovations, changing `@exception` to `@throws`
probably gives the least bang for the buck. If nothing else, it gives you 3
extra characters on the same line to fill with something more useful.
I would be more inclined to change `...` to `{@code ...}`, but
given how error-prone that can be, I still wouldn't do it in this changeset.
-Pavel
> On 8 Apr 2020, at 13:56, David Holmes wrote:
>
> Hi Pavel,
>
> Not a review ...
>
> On 8/04/2020 9:50 pm, Pavel Rappo wrote:
>> Vipin, here you go:
>> https://bugs.openjdk.java.net/browse/JDK-8242366
>> http://cr.openjdk.java.net/~prappo/8242366/webrev.00/
>> I took the liberty of additionally fixing a couple of parameters' names,
>> a typo, and `@exception` tags for checked exceptions that were neither thrown
>> nor imported.
>
> While you are in there is it worth changing @exception to @throws? (I didn't
> look to see how big that change would be.)
>
> Cheers,
> David
>
>> The bulk of the change is in Security. Some changes are in Networking. The
>> appropriate mailing lists are in CC for this email. We should wait for their
>> feedback.
>> Changes in core area look good to me and I'd be surprised if there are any
>> problems with the remaining portion of the changeset.
>> -Pavel
>>> On 7 Apr 2020, at 19:50, Vipin Sharma wrote:
>>>
>>> Hi Pavel,
>>>
>>>> On Apr 7, 2020, at 11:11 PM, Pavel Rappo wrote:
>>>>
>>>> I assume you have signed the OCA [1]. If not and you want to continue,
>>>> please do it. If you've already done so, which is probably the case [2],
>>>> please attach your patch as text to this thread with the next email. Do
>>>> not use zip or the like. I will take it from there and sponsor that for
>>>> you.
>>> Yes I have signed OCA.
>>>>
>>>> -Pavel
>>>>
>>>> [1] https://www.oracle.com/technetwork/community/oca-486395.html
>>>> [2] changeset: 58344:65f30e209890
>>>> user:clanger
>>>> date:Wed Mar 11 13:50:13 2020 +0100
>>>> files: test/jdk/java/lang/Boolean/GetBoolean.java
>>>> test/jdk/java/lang/Boolean/MakeBooleanComparable.java
>>>> test/jdk/java/lang/Boolean/ParseBoolean.java
>>>> description:
>>>> 8240524: Remove explicit type argument in test
>>>> jdk/java/lang/Boolean/MakeBooleanComparable.java
>>>> Reviewed-by: clanger, vtewari
>>>> Contributed-by: vipinsharma85 at gmail.com
>>>>
>>> Yes this is my first contribution.
>>>
>>> Patch text:
>>>
>>> --- old/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java
>>> 2020-04-06 00:19:10.546117441 +0530
>>> +++ new/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java
>>> 2020-04-06 00:19:10.130115855 +0530
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or modify it
>>> @@ -202,7 +202,7 @@
>>> /**
>>> * Sets the padding mechanism of this cipher.
>>> *
>>> - * @param padding the padding mechanism
>>> + * @param paddingScheme the padding mechanism
>>> *
>>> * @exception NoSuchPaddingException if the requested padding mechanism
>>> * does not exist
>>> ---
>>> old/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java
>>> 2020-04-06 00:19:11.526121179 +0530
>>> +++
>>> new/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java
>>> 2020-04-06 00:19:11.118119622 +0530
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2004, 2017, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEA
Re: Need sponsor to fix Javadoc warnings
If your new patch addresses a similar type of problem, please send it in reply to this email, so that I could merge it with the existing patch. Let's try to minimize process overhead if possible. > On 8 Apr 2020, at 17:35, Vipin Sharma wrote: > > > >> On Apr 8, 2020, at 6:57 PM, Pavel Rappo wrote: >> >> Why assume something that sophisticated where it can be adequately explained >> by >> a simpler thing? :) I bet it was an IDE inspection. >> >> -Pavel > > Yes, it was IDE inspection in java.base, it looked like the best way to start > contributing and understand the process. > If it helps and not creating noise for you all, I see an opportunity for one > more such patch. What do you think? > >> >>> On 8 Apr 2020, at 14:14, Alan Bateman wrote: >>> >>> On 08/04/2020 14:07, Daniel Fuchs wrote: >>>> Hi Pavel, >>>> >>>> On 08/04/2020 13:56, David Holmes wrote: >>>>> and `@exception` tags for checked exceptions that were neither thrown >>>>> nor imported >>>> >>>> Hopefully that's only on internal classes. >>> From a quick scan, the changes are to internal classes and a few non-public >>> elements of public API classes. So I don't think anything should impact the >>> javadoc. I'm guessing this patch is motivated by something creating that is >>> running the javadoc tool with different options to what the "docs" target >>> uses. >>> >>> -Alan >> > > Regards, > Vipin
Re: Need sponsor to fix Javadoc warnings
Vipin,
I saw that Max had already reviewed that incremental patch. That's good.
I couldn't resist fixing a couple of typos in the already affected
jdk.internal.icu
(International Components for Unicode) package. Once this has been cleared by
experts in that area, we are good to go.
Here's the cumulative webrev:
http://cr.openjdk.java.net/~prappo/8242366/webrev.01/
-Pavel
> On 11 Apr 2020, at 20:23, Vipin Sharma wrote:
>
> Hi Pavel,
>
>> On Apr 9, 2020, at 2:45 AM, Pavel Rappo wrote:
>>
>> If your new patch addresses a similar type of problem, please send it in
>> reply to this email,
>> so that I could merge it with the existing patch. Let's try to minimize
>> process overhead if possible.
>>
> This is additional patch:
>
> --- old/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java
> 2020-04-12 00:33:54.818724363 +0530
> +++ new/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java
> 2020-04-12 00:33:54.398714466 +0530
> @@ -142,7 +142,7 @@
>/**
> * Called by com.ibm.icu.util.Trie to extract from a lead surrogate's
> * data the index array offset of the indexes for that lead surrogate.
> -* @param property data value for a surrogate from the trie, including
> +* @param value data value for a surrogate from the trie, including
> *the folding offset
> * @return data offset or 0 if there is no data for the lead surrogate
> */
> ---
> old/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
> 2020-04-12 00:33:55.778746974 +0530
> +++
> new/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
> 2020-04-12 00:33:55.346736801 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1996, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -39,7 +39,7 @@
>
> /**
> * Calls FilterInputStream's constructor.
> - * @param an InputStream
> + * @param is an InputStream
> */
> PlainTextInputStream(InputStream is) {
> super(is);
> ---
> old/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
> 2020-04-12 00:33:56.726769287 +0530
> +++
> new/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
> 2020-04-12 00:33:56.306759403 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2012, 2017, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -881,7 +881,7 @@
> * only CRLs signed with a different key (but the same issuer
> * name) as the certificate being checked.
> *
> - * @param currCert the X509Certificate to be checked
> + * @param cert the X509Certificate to be checked
> * @param prevKey the PublicKey that failed
> * @param signFlag true if that key was trusted to sign CRLs
> * @param stackedCerts a Set of
> X509Certificates>
> ---
> old/src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
> 2020-04-12 00:33:57.658791207 +0530
> +++
> new/src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
> 2020-04-12 00:33:57.250781612 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2006, 2019, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -166,7 +166,7 @@
> /**
> * Creates a URICertStore.
> *
> - * @param parameters specifying the URI
> + * @param params parameters specifying the URI
> */
> URICertStore(CertStoreParameters params)
> throws InvalidAlgorithmParameterException, NoSuchAlgorithmException {
> ---
> old/src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java
> 2020-04-12 00:33:58.602813394 +0530
> +++
> new/src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java
> 2020-04-12 00:33:58.178803431 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2015, 2018, Oracle and/or its affili
Re: Need sponsor to fix Javadoc warnings
Vipin,
After a private exchange with Naoto Sato, who is fluent in that area, I decided
to leave out all the changes to the jdk.internal.icu package from the changeset.
The reason is quite simple. A significant portion of code in jdk.internal.icu
comes from an upstream project, ICU4J. Making OpenJDK-local changes to it makes
it harder to merge when updating from upstream.
Before I created that latter webrev I found that there were OpenJDK-local
changes to that package. Still, the argument holds and we should not aggravate
already existing difficulties.
-Pavel
P.S. If you care about those changes, you may want to ask ICU4J [^1] folk to
incorporate them. If they agree, one day those changes may show up in the
OpenJDK.
--
[^1]: https://github.com/unicode-org/icu/tree/master/icu4j
> On 15 Apr 2020, at 12:06, Pavel Rappo wrote:
>
> Vipin,
>
> I saw that Max had already reviewed that incremental patch. That's good.
>
> I couldn't resist fixing a couple of typos in the already affected
> jdk.internal.icu
> (International Components for Unicode) package. Once this has been cleared by
> experts in that area, we are good to go.
>
> Here's the cumulative webrev:
>
>http://cr.openjdk.java.net/~prappo/8242366/webrev.01/
>
> -Pavel
>
>> On 11 Apr 2020, at 20:23, Vipin Sharma wrote:
>>
>> Hi Pavel,
>>
>>> On Apr 9, 2020, at 2:45 AM, Pavel Rappo wrote:
>>>
>>> If your new patch addresses a similar type of problem, please send it in
>>> reply to this email,
>>> so that I could merge it with the existing patch. Let's try to minimize
>>> process overhead if possible.
>>>
>> This is additional patch:
>>
>> --- old/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java
>> 2020-04-12 00:33:54.818724363 +0530
>> +++ new/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java
>> 2020-04-12 00:33:54.398714466 +0530
>> @@ -142,7 +142,7 @@
>> /**
>>* Called by com.ibm.icu.util.Trie to extract from a lead surrogate's
>>* data the index array offset of the indexes for that lead surrogate.
>> -* @param property data value for a surrogate from the trie,
>> including
>> +* @param value data value for a surrogate from the trie, including
>>*the folding offset
>>* @return data offset or 0 if there is no data for the lead surrogate
>>*/
>> ---
>> old/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
>>2020-04-12 00:33:55.778746974 +0530
>> +++
>> new/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
>>2020-04-12 00:33:55.346736801 +0530
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 1996, Oracle and/or its affiliates. All rights reserved.
>> + * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights
>> reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> @@ -39,7 +39,7 @@
>>
>>/**
>> * Calls FilterInputStream's constructor.
>> - * @param an InputStream
>> + * @param is an InputStream
>> */
>>PlainTextInputStream(InputStream is) {
>>super(is);
>> ---
>> old/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>> 2020-04-12 00:33:56.726769287 +0530
>> +++
>> new/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>> 2020-04-12 00:33:56.306759403 +0530
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2012, 2017, Oracle and/or its affiliates. All rights
>> reserved.
>> + * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights
>> reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> @@ -881,7 +881,7 @@
>> * only CRLs signed with a different key (but the same issuer
>> * name) as the certificate being checked.
>> *
>> - * @param currCert the X509Certificate to be checked
>> + * @param cert the X509Certificate to be checked
>> * @param prevKey the PublicKey that failed
>> * @param signFlag true if that key was trusted to sign CRLs
>> * @param stackedCerts a Set of
>> X509Certificates>
>> ---
>> old/src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
>> 2020-0
RFR: 8274075: Fix miscellaneous typos in java.base
8274075: Fix miscellaneous typos in java.base - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5610/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274075 Stats: 21 lines in 8 files changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Tweak wording for Throwable constructor parameters - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/2db8525d..19b6f496 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module
On Thu, 16 Sep 2021 19:03:26 GMT, Andrey Turbanov wrote: > Pass "cause" exception as constructor parameter is shorter and easier to read. This will need to be thoroughly tested to make sure there were no implicit dependencies on now-removed happens-before edges (`initCause` is synchronized). That said, the idea behind this clean-up looks good. - PR: https://git.openjdk.java.net/jdk/pull/5551
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 16:56:03 GMT, Lance Andersen wrote: >> src/java.base/share/classes/java/lang/Throwable.java line 68: >> >>> 66: * Further, doing so would tie the API of the upper layer to the >>> details of >>> 67: * its implementation, assuming the lower layer's exception was a >>> checked >>> 68: * exception. Throwing a "wrapping exception" (i.e., an exception >>> containing a >> >> Are we sure that this should be "wrapping" and not "wrapped?" See also for >> example `java.security.cert.CertPathValidatorException.` > > It does seem a bit strange to say "Throwing a wrapping" If we have two exceptions A and B, such that B is the cause of A, then A is the wrapping exception (the one that wraps or the wrapper) and B is the wrapped exception (the one that is being wrapped or the wrappee). I noticed that the sentence was conflicting: it started with the "wrapped" exception, but then in the "i.e." commentary in parentheses proceeded with the wrappee exception. To resolve that conflict, I proposed to rephrase the first part of that sentence. FWIW, my original suggestion elsewhere was to use "wrapper". - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 17:10:02 GMT, Brian Burkhalter wrote: >> If we have two exceptions A and B, such that B is the cause of A, then A is >> the wrapping exception (the one that wraps or the wrapper) and B is the >> wrapped exception (the one that is being wrapped or the wrappee). >> >> I noticed that the sentence was conflicting: it started with the "wrapped" >> exception, but then in the "i.e." commentary in parentheses proceeded with >> the wrappee exception. To resolve that conflict, I proposed to rephrase the >> first part of that sentence. FWIW, my original suggestion elsewhere was to >> use "wrapper". > > Subjectively, "wrapping exception" would seem to be an exception in the > process of wrapping something. Would "wrappER" be better? - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 17:14:31 GMT, Pavel Rappo wrote: >> Subjectively, "wrapping exception" would seem to be an exception in the >> process of wrapping something. > > Would "wrappER" be better? We can either revert this part of the change or rephrase it. Mind you, rephrasing might prove tricky because of non-local changes it might introduce. There's one more occurrence of "wrapped exception" in this file: https://github.com/openjdk/jdk/blob/19b6f49649a317e6b255aeb810c8d533119b93bb/src/java.base/share/classes/java/lang/Throwable.java#L548-L554 - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 22:00:29 GMT, David Holmes wrote: >> Instead of "common case where a wrapped exception is thrown from same >> method" could one write "common case where an enclosing exception is thrown >> from the same method"? > > Note that we don't throw the "wrapped exception" we throw the exception that > wraps it. The "wrapped exception" is the original cause. The wording as > presented now is correct in that regard. You could also say "Throwing a > wrapper exception (i.e. one that has a cause)" - I think both are > grammatically correct. > > The later text "where a wrapped exception is thrown from the same method" is > again incorrect because the wrapped exception (the cause) is not what gets > thrown. But I find that whole sentence rather jarring anyway - I'm not > certain exactly what it means. I'm inclined to revert this change on Throwable.java:68, because of the lack of consensus. It clearly is not a simple typo. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Wed, 22 Sep 2021 10:24:12 GMT, Pavel Rappo wrote: >> Note that we don't throw the "wrapped exception" we throw the exception that >> wraps it. The "wrapped exception" is the original cause. The wording as >> presented now is correct in that regard. You could also say "Throwing a >> wrapper exception (i.e. one that has a cause)" - I think both are >> grammatically correct. >> >> The later text "where a wrapped exception is thrown from the same method" is >> again incorrect because the wrapped exception (the cause) is not what gets >> thrown. But I find that whole sentence rather jarring anyway - I'm not >> certain exactly what it means. > > I'm inclined to revert this change on Throwable.java:68, because of the lack > of consensus. It clearly is not a simple typo. Reverted in 57b71c5. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 13:16:02 GMT, Pavel Rappo wrote: >> 8274075: Fix miscellaneous typos in java.base > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Tweak wording for Throwable constructor parameters Please re-review this change since I've added two commits. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v3]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with two additional commits since the last revision: - Fix "non-white space" JDK predominantly uses "non-whitespace". There's only one more use of "non-white space", in com/sun/tools/javac/tree/DocTreeMaker.java:716, which will go away soon. - Undo "wrapped" -> "wrapping" - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/19b6f496..05505d97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v4]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Add missing "the" (Spotted by Brian Burkhalter.) - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/05505d97..fa81cacd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5610&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Integrated: 8274075: Fix miscellaneous typos in java.base
On Tue, 21 Sep 2021 12:26:25 GMT, Pavel Rappo wrote: > 8274075: Fix miscellaneous typos in java.base This pull request has now been integrated. Changeset: 87998565 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/8799856528f5804b616b734caed3fc4ba9811bfa Stats: 26 lines in 9 files changed: 0 ins; 1 del; 25 mod 8274075: Fix miscellaneous typos in java.base Reviewed-by: dfuchs, darcy, iris, lancea, bpb - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274809: Update java.base classes to use try-with-resources
On Wed, 6 Oct 2021 16:00:41 GMT, Bradford Wetmore wrote:
>> 8274809: Update java.base classes to use try-with-resources
>
> src/java.base/share/classes/sun/security/timestamp/HttpTimestamper.java line
> 127:
>
>> 125: // Receive the reply
>> 126: byte[] replyBuffer = null;
>> 127: try (BufferedInputStream input = new
>> BufferedInputStream(connection.getInputStream())) {
>
> Same comment as above.
In this and the immediately preceding case, it might be better to use `var`.
-
PR: https://git.openjdk.java.net/jdk/pull/5818
RFR: 8276348: Use blessed modifier order in java.base
This PR follows up one of the recent PRs, where I used a non-canonical modifier order. Since the problem was noticed [^1], why not to address it at mass? As far as I remember, the first mass-canonicalization of modifiers took place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 files. Since then modifiers have become a bit lose, and it makes sense to re-bless (using the JDK-8136583 terminology) them. This change was produced by running the below command followed by updating the copyright years on the affected files where necessary: $ sh ./bin/blessed-modifier-order.sh src/java.base The resulting change is much smaller than that of 2015: 39 lines spanning 21 files. [^1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) [^2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6213/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6213&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276348 Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/6213.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6213/head:pull/6213 PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html A colleague suggested that I should clarify that the `blessed-modifier-order.sh` script that I used is available in the JDK repo at https://github.com/openjdk/jdk/blob/01105d6985b39d4064b9066eab3612da5a401685/bin/blessed-modifier-order.sh. That script was contributed by Martin Buchholz in JDK-8136656 in 2015. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:39:17 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/lang/Object.java line 282:
>>
>>> 280: * For objects of type {@code Class,} by executing a
>>> 281: * static synchronized method of that class.
>>> 282: *
>>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
> I think Roger is right and maybe the change to the javadoc could be dropped
> from this patch.
It's tough when a natural language clashes with a programming language. I
appreciate that this particular clash might cause discomfort to native English
speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective order.)
That said, consider the following pragmatic aspect. Unless we change the script
not to process prose in comments (btw, how would we do that?), every single
time we run that script, that particular line in Object.java will need to be
tracked and excluded.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:45:07 GMT, Pavel Rappo wrote: >>> In comments, I think the 'synchronized static 'reads better, the >>> conventional order is for the code. >> >> I think Roger is right and maybe the change to the javadoc could be dropped >> from this patch. > > It's tough when a natural language clashes with a programming language. I > appreciate that this particular clash might cause discomfort to native > English speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective > order.) That said, consider the following pragmatic aspect. Unless we change > the script not to process prose in comments (btw, how would we do that?), > every single time we run that script, that particular line in Object.java > will need to be tracked and excluded. Here's a bit of archaeology. I found the original JDK-8136583 patch, which has moved from where it was in the RFR to https://cr.openjdk.java.net/~martin/webrevs/jdk9/blessed-modifier-order/. That patch doesn't change Object.java. The RFR thread mentions neither that javadoc change nor any javadoc change for that matter. So either the script was different, or Martin filtered that line (from Object.java) out before creating the webrev. Now, in his followup thread on core-libs-dev, http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035273.html, Martin specifically pointed out javadoc changes and said that they seem fine to him. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 18:48:20 GMT, Roger Riggs wrote:
> Pragmatically, fix the script to ignore those keywords on comment lines.
> Learn Perl, its just a regular expression pattern match and replace
> expression.
I understand in principle how to modify that script to ignore doc comments. The
thing I was referring to when said "btw, how would we do that?" was this: not
all comment lines are prose. Some of those lines belong to snippets of code,
which I guess you would also like to be properly formatted.
> But having seen several reviewers be unmoved by the difference, the real
> pragmatic view is to ignore the English.
I'm sorry you feel that way. Would it be okay if I made it clear that those two
words are not English adjectives but are special symbols that happen to use
Latin script and originate from the English words they resemble? If so, I could
enclose each of them in `{@code ... }`. If not, I could drop that particular
change from this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:15:26 GMT, Andrey Turbanov wrote:
>> This PR follows up one of the recent PRs, where I used a non-canonical
>> modifier order. Since the problem was noticed [^1], why not to address it at
>> mass?
>>
>> As far as I remember, the first mass-canonicalization of modifiers took
>> place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning
>> 453 files. Since then modifiers have become a bit loose, and it makes sense
>> to re-bless (using the JDK-8136583 terminology) them.
>>
>> This change was produced by running the below command followed by updating
>> the copyright years on the affected files where necessary:
>>
>> $ sh ./bin/blessed-modifier-order.sh src/java.base
>>
>> The resulting change is much smaller than that of 2015: 39 lines spanning 21
>> files.
>>
>> [^1]:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
>> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
>> [^2]:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
>
> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>
>> 86: */
>> 87: public
>> 88: abstract class CallSite {
>
> I think it's better to move all modifiers to the single line.
This is a survivorship bias. This example jumps out at you, because it happens
to use missorted modifiers. I'm pretty sure this is not an aberration, but a
style. If you want to change that style, you should create a respective JBS
issue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:22:15 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>>
>>> 86: */
>>> 87: public
>>> 88: abstract class CallSite {
>>
>> I think it's better to move all modifiers to the single line.
>
> This is a survivorship bias. This example jumps out at you, because it
> happens to use missorted modifiers. I'm pretty sure this is not an
> aberration, but a style. If you want to change that style, you should create
> a respective JBS issue.
I've grepped the code and can now see that a list of modifiers broken by
newlines which cannot be explained by line-width concerns is indeed an
irregularity. Not only in java.base but also in other modules. Although there
aren't many of such cases, I would prefer them to be addressed in a separate
cleanup, which you are welcome to pursue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Integrated: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it en > masse? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html This pull request has now been integrated. Changeset: 61506336 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/615063364ab6bdd3fa83401745e05b45e13eacdb Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod 8276348: Use blessed modifier order in java.base Reviewed-by: dfuchs, darcy, iris, rriggs, martin - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:
>>> Pragmatically, fix the script to ignore those keywords on comment lines.
>>> Learn Perl, its just a regular expression pattern match and replace
>>> expression.
>>
>> I understand in principle how to modify that script to ignore doc comments.
>> The thing I was referring to when said "btw, how would we do that?" was
>> this: not all comment lines are prose. Some of those lines belong to
>> snippets of code, which I guess you would also like to be properly formatted.
>>
>>> But having seen several reviewers be unmoved by the difference, the real
>>> pragmatic view is to ignore the English.
>>
>> I'm sorry you feel that way. Would it be okay if I made it clear that those
>> two words are not English adjectives but are special symbols that happen to
>> use Latin script and originate from the English words they resemble? If so,
>> I could enclose each of them in `{@code ... }`. If not, I could drop that
>> particular change from this PR.
>
> The blessed-modifier-order.sh script intentionally modifies comments, with
> the hope of finding code snippets (it did!)
>
> Probably I manually deleted the change to Object.java back in 2015, to avoid
> the sort of controversy we're seeing now.
> I don't have a strong feeling either way on changing that file.
>
> I agree with @pavelrappo that script-generated changes should not be mixed
> with manual changes.
> I would also not update copyright years for such changes.
>
> It's a feature of blessed-modifier-order.sh that all existing formatting is
> perfectly preserved.
One more thing. Please have a look at this other line in the same file; this
line was there before the change
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it
consistent. **If one is going to ever revert that controversial part of the
change, please update both lines so that the file remains consistent.**
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276401: Use blessed modifier order in java.net.http
On Wed, 3 Nov 2021 10:11:31 GMT, Daniel Fuchs wrote: > Hi, > > Please find here a trivial cleanup change that updates classes in the > `java.net.http` module to use the "blessed modifier order". > > The changeset was obtained by running `sh ./bin/blessed-modifier-order.sh > src/java.net.http`. > > best regards, > > -- daniel Don't forget to update the copyright years, if you think that it's necessary. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6228
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical
> modifier order. Since the problem was noticed [^1], why not to address it en
> masse?
>
> As far as I remember, the first mass-canonicalization of modifiers took place
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
> files. Since then modifiers have become a bit loose, and it makes sense to
> re-bless (using the JDK-8136583 terminology) them.
>
> This change was produced by running the below command followed by updating
> the copyright years on the affected files where necessary:
>
> $ sh ./bin/blessed-modifier-order.sh src/java.base
>
> The resulting change is much smaller than that of 2015: 39 lines spanning 21
> files.
>
> [^1]:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
> [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_
>
> On 3/11/2021 9:26 pm, Pavel Rappo wrote:
>
> > On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz
> > wrote:
> > > > > Pragmatically, fix the script to ignore those keywords on comment
> > > > > lines. Learn Perl, its just a regular expression pattern match and
> > > > > replace expression.
> > > >
> > > >
> > > > I understand in principle how to modify that script to ignore doc
> > > > comments. The thing I was referring to when said "btw, how would we do
> > > > that?" was this: not all comment lines are prose. Some of those lines
> > > > belong to snippets of code, which I guess you would also like to be
> > > > properly formatted.
> > > > > But having seen several reviewers be unmoved by the difference, the
> > > > > real pragmatic view is to ignore the English.
> > > >
> > > >
> > > > I'm sorry you feel that way. Would it be okay if I made it clear that
> > > > those two words are not English adjectives but are special symbols that
> > > > happen to use Latin script and originate from the English words they
> > > > resemble? If so, I could enclose each of them in `{@code ... }`. If
> > > > not, I could drop that particular change from this PR.
> > >
> > >
> > > The blessed-modifier-order.sh script intentionally modifies comments,
> > > with the hope of finding code snippets (it did!)
> > > Probably I manually deleted the change to Object.java back in 2015, to
> > > avoid the sort of controversy we're seeing now.
> > > I don't have a strong feeling either way on changing that file.
> > > I agree with @pavelrappo that script-generated changes should not be
> > > mixed with manual changes.
> > > I would also not update copyright years for such changes.
> > > It's a feature of blessed-modifier-order.sh that all existing formatting
> > > is perfectly preserved.
> >
> >
> > One more thing. Please have a look at this other line in the same file;
> > this line was there before the change
> > https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
> > So before the change, the file was somewhat inconsistent. The change made
> > it consistent. **If one is going to ever revert that controversial part of
> > the change, please update both lines so that the file remains consistent.**
>
> Line 281 is (was!) consistent with line 277 because it is distinguishing a
> synchronized "static method" from a synchronized "instance method". There was
> no need to make any change to line 281 because of the blessed-order of
> modifiers as defined for method declarations! This text is just prose. Now
> for consistency you should change line 277 to refer to a "non-static
> synchronized method" (as "instance synchronized method" would be truly awful).
Thanks, David. You've provided a clear and convincing argument, and I can see
the inconsistency I introduced. I can revert that particular piece back if you
think that it would be appropriate.
That said, this line will have to be filtered out every time the script is run.
I could probably provide a more involved script that harnesses the power of AST
(com.sun.source.doctree) to try to filter o
RFR: 8279918: Fix various doc typos
- Most of the typos are of a trivial kind: missing whitespace. - If any of the typos should be fixed in the upstream projects instead, please say so; I will drop those typos from the patch. - As I understand it, in ImageInputStream and DataInput is an irrelevant formatting artefact and thus can be removed. - ' is an apostrophe, which does not require to be encoded. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/7063/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7063&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279918 Stats: 85 lines in 34 files changed: 0 ins; 0 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/7063.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7063/head:pull/7063 PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:46:11 GMT, Kevin Walls wrote: >> - Most of the typos are of a trivial kind: missing whitespace. >> - If any of the typos should be fixed in the upstream projects instead, >> please say so; I will drop those typos from the patch. >> - As I understand it, in ImageInputStream and DataInput is an irrelevant >> formatting artefact and thus can be removed. >> - ' is an apostrophe, which does not require to be encoded. > > src/java.base/share/classes/sun/text/RuleBasedBreakIterator.java line 73: > >> 71: * will be transparent to the BreakIterator. A sequence of >> characters will break the >> 72: * same way it would if any ignore characters it contains are taken >> out. Break >> 73: * positions never occur before ignore characters. > > "before ignored characters" I believe it's the name of a concept, so I will leave it as is: > There is one special substitution. If the description defines a substitution > called "", the expression must be a [] expression, and the expression > defines a set of characters (the "ignore characters") that will be > transparent to the BreakIterator. - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:00:55 GMT, Kevin Walls wrote:
>> - Most of the typos are of a trivial kind: missing whitespace.
>> - If any of the typos should be fixed in the upstream projects instead,
>> please say so; I will drop those typos from the patch.
>> - As I understand it, in ImageInputStream and DataInput is an irrelevant
>> formatting artefact and thus can be removed.
>> - ' is an apostrophe, which does not require to be encoded.
>
> src/java.sql/share/classes/java/sql/BatchUpdateException.java line 58:
>
>> 56: * A JDBC driver implementation should use
>> 57: * the constructor {@code BatchUpdateException(String reason, String
>> SQLState,
>> 58: * int vendorCode, long []updateCounts, Throwable cause) } instead of
>
> While we are here, is it more normal to say "long[] updateCounts", not
> separating the [] from the type.
> Similarly at line 81, 118, 151, 247, 277, 318, 354.
I thought about it too, but then noticed how the position of `[]` mimics that
of the respective method signatures in code.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:18:42 GMT, Kevin Walls wrote:
>> - Most of the typos are of a trivial kind: missing whitespace.
>> - If any of the typos should be fixed in the upstream projects instead,
>> please say so; I will drop those typos from the patch.
>> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant
>> formatting artefact and thus can be removed.
>> - `'` is an apostrophe, which does not require to be encoded.
>
> src/java.sql/share/classes/java/sql/Statement.java line 784:
>
>> 782: * statement returns a {@code ResultSet} object, the second argument
>> 783: * supplied to this method is not an
>> 784: * {@code int} array whose elements are valid column indexes, the
>> method is called on a
>
> Should it be "or the method is called on...", i.e. add the "or", otherwise
> it's a list of problems but we don't know if they are all required, or are
> alternatives. It probably does not mean that all these have to be true to
> throw the exception, but it doesn't say that. We are nitpicking of course.
You are right in that this `@throws` description reads a bit weird in its
current form. That said, I wouldn't touch it in this PR for two reasons.
Firstly, this wording seems to be consistent with other locations in that file.
Secondly, this is a spec territory.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:42:48 GMT, Lance Andersen wrote: > The above is a bit confusing as it reads(same in ImageInputStream.java). I > think that that can be looked at later. Just to clarify: you are okay with removing the ` ` entity, right? As for ImageInputStream, some doc comments should probably use `@inheritDoc`. But this is a much bigger clean-up. - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:38:13 GMT, Lance Andersen wrote: > Yes an "or" is probably worthwhile to add. I would prefer to leave it for the follow-up cleanup if only because adding "or" here would make it inconsistent with other `@throws SQLException` descriptions in that file and perhaps elsewhere in java.sql: * java/sql/Statement.java:59 * java/sql/Statement.java:85 * java/sql/Statement.java:346 * java/sql/Statement.java:524 * java/sql/Statement.java:745 * java/sql/Statement.java:814 * java/sql/Statement.java:860 * java/sql/Statement.java:913 * java/sql/Statement.java:962 * java/sql/Statement.java:1225 * java/sql/Statement.java:1269 * java/sql/Statement.java:1314 - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos [v2]
> - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - `'` is an apostrophe, which does not require to be encoded. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Additional typos - Changes: - all: https://git.openjdk.java.net/jdk/pull/7063/files - new: https://git.openjdk.java.net/jdk/pull/7063/files/fe81eeca..b256a13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7063&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7063&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7063.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7063/head:pull/7063 PR: https://git.openjdk.java.net/jdk/pull/7063
Integrated: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote: > - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - `'` is an apostrophe, which does not require to be encoded. This pull request has now been integrated. Changeset: f1805309 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/f1805309352a22119ae2edf8bfbb596f00936224 Stats: 88 lines in 35 files changed: 0 ins; 0 del; 88 mod 8279918: Fix various doc typos Reviewed-by: kevinw, lancea, mullan, sspitsyn, naoto, jlahoda, azvegint, egahlin, jjg - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. src/java.base/share/legal/icu.md line 310: > 308: # list of conditions and the following disclaimer. Redistributions in > binary > 309: # form must reproduce the above copyright notice, this list of > conditions and > 310: # the following disclaimer in the documentation and/or the materials I think it's a mistype of "other", not "the"; look at the similar text below. src/java.base/share/native/libzip/zlib/ChangeLog line 99: > 97: - Simplify contrib/vstudio/vc10 with 'd' suffix > 98: - Add TOP support to win32/Makefile.msc > 99: - Support i686 and amd64 assembler builds in CMakeLists.txt Similarly to Naoto's comment on ICU: shouldn't we leave this as is? If anybody cares enough about this typo, they could file a bug against zlib directly. src/java.base/share/native/libzip/zlib/README line 67: > 65: when compiled with cc. > 66: > 67: - On Digital Unix 4.0D (formerly OSF/1) on AlphaServer, the cc option > -std1 is Same as above. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base
On Fri, 15 Apr 2022 11:25:09 GMT, Pavel Rappo wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > src/java.base/share/legal/icu.md line 310: > >> 308: # list of conditions and the following disclaimer. Redistributions in >> binary >> 309: # form must reproduce the above copyright notice, this list of >> conditions and >> 310: # the following disclaimer in the documentation and/or the materials > > I think it's a mistype of "other", not "the"; look at the similar text below. Should be addressed on the ICU side. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. I note that many years ago you filed JDK-6327933, which I'm currently looking at. If implemented, many of the issues from this PR will be addressed automatically, including those in `java.util.concurrent`. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: Replace concat String to append in StringBuilder parameters
> In the class
> src/share/classes/javax/management/openmbean/CompositeType.java you have
> added the
> annotation @SuppressWarnings("StringConcatenationInsideStringBufferAppend")
> instead of fixing the concatenation inside the append method. Why?
+1 Moreover, I wonder where this value comes from? I've never seen it before.
Here are warnings that javac supports:
all,auxiliaryclass,cast,classfile,deprecation,dep-ann,divzero,empty,fallthrough,finally,options,overloads,overrides,path,processing,rawtypes,serial,static,try,unchecked,varargs
It doesn't look like one of Eclipse's warnings either.
> And I would like to suggest to drop explicit usage of StringBuilder in some
> methods at all to improve code readability.
Agree.
-Pavel
Re: Replace concat String to append in StringBuilder parameters
Otavio,
Just skimmed through your changes. It looks good. But there are some things we
can make a little bit better though. IMO, it's not always a performance that
matters (looking around to see if Alexey Shipilev is somewhere near) but
readability. It's good to estimate performance requirements of a code before
making a decision of whether to sacrifice readability for performance or not. I
noticed there are some methods that are screaming for readability improvements.
Namely, different versions of joining strings through a separator. You can find
them in following methods in your changeset:
sun.security.acl.AclEntryImpl::toString
sun.security.ssl.HandshakeMessage.CertificateRequest::print
javax.swing.MultiUIDefaults::toString (one of the most bizarre one)
sun.security.provider.PolicyFile::expandSelf
java.security.ProtectionDomain::toString
javax.management.relation.Role::toString
sun.security.ssl.SupportedEllipticCurvesExtension::toString
sun.tools.jstat.SyntaxException::SyntaxException
Unfortunately, neither java.util.StringJoiner nor String.join have perfect (but
who has?) APIs. So it's up to us to figure out the best way of joining
elements. So when the power of those guys is not enough, we can go this way (or
maybe some else):
Iterable elements = ...
StringJoiner j = new StringJoiner(", ");
elements.forEach(e -> j.add(e.toString()));
or
Collection elements = ...
String i = elements.stream()
.map(Object::toString)
.collect(Collectors.joining(", "));
Other than that:
1. It looks like some StringBuffers are still lurking out there. We should
change them to StringBuilders, *only if* we can prove they are not exposed to
more than one thread:
--- dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java
(revision 10452+:8e501e6bbf1f+)
@@ -272,7 +272,7 @@
@Override
public String toString() {
-StringBuffer sb = new StringBuffer();
+StringBuilder sb = new StringBuilder();
//$$fb2002-11-01: fix for 4672864: AudioFileFormat.toString() throws
unexpected NullPointerException
if (type != null) {
---
dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++
dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java
(revision 10452+:8e501e6bbf1f+)
@@ -346,12 +346,12 @@
public String valueString(boolean bigLimit) {
// Char arrays deserve special treatment
-StringBuffer result;
+StringBuilder result;
byte[] value = getValue();
int max = value.length;
byte elementSignature = getElementType();
if (elementSignature == 'C') {
-result = new StringBuffer();
+result = new StringBuilder();
for (int i = 0; i < value.length; ) {
char val = charAt(i, value);
result.append(val);
@@ -362,7 +362,7 @@
if (bigLimit) {
limit = 1000;
}
-result = new StringBuffer("{");
+result = new StringBuilder("{");
int num = 0;
for (int i = 0; i < value.length; ) {
if (num > 0) {
--- dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision
10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision
10452+:8e501e6bbf1f+)
@@ -1213,7 +1213,7 @@
registerComponents(horizontalGroup, HORIZONTAL);
registerComponents(verticalGroup, VERTICAL);
}
-StringBuffer buffer = new StringBuffer();
+StringBuilder buffer = new StringBuilder();
buffer.append("HORIZONTAL\n");
createSpringDescription(buffer, horizontalGroup, " ", HORIZONTAL);
buffer.append("\nVERTICAL\n");
@@ -1221,7 +1221,7 @@
return buffer.toString();
}
-private void createSpringDescription(StringBuffer buffer, Spring spring,
+private void createSpringDescription(StringBuilder buffer, Spring spring,
String indent, int axis) {
String origin = "";
String padding = "";
--- dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java
(revision 10452+:8e501e6bbf1f+)
@@ -463,7 +463,7 @@
// -
-private static void appendIfLiteralAddress(String addr, StringBuffer sb) {
+private static void appendIfLiteralAddress(String addr, StringBuilder sb) {
if (IPAddressUtil.isIPv4LiteralAddress(addr))
Re: Replace concat String to append in StringBuilder parameters
It's exactly the way it's been working since 1.6 I believe.
public class Optimization {
public String concat(String... strings) {
return "#: " + strings[0] + strings[2] + strings[3] + "...";
}
}
public class Optimization {
public Optimization();
Code:
0: aload_0
1: invokespecial #1 // Method
java/lang/Object."":()V
4: return
public java.lang.String concat(java.lang.String...);
Code:
0: new #2 // class java/lang/StringBuilder
3: dup
4: invokespecial #3 // Method
java/lang/StringBuilder."":()V
7: ldc #4 // String #:
9: invokevirtual #5 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
12: aload_1
13: iconst_0
14: aaload
15: invokevirtual #5 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
18: aload_1
19: iconst_2
20: aaload
21: invokevirtual #5 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
24: aload_1
25: iconst_3
26: aaload
27: invokevirtual #5 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
30: ldc #6 // String ...
32: invokevirtual #5 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
35: invokevirtual #7 // Method
java/lang/StringBuilder.toString:()Ljava/lang/String;
38: areturn
}
-Pavel
On 26 Aug 2014, at 06:20, Xuelei Fan wrote:
> I was wondering, is it nice to address it in Java compiler to use string
> builder for the string "+" operator?
>
> Xuelei
>
>
> On 8/26/2014 11:28 AM, Wang Weijun wrote:
>> New webrevs available at
>>
>> http://cr.openjdk.java.net/~weijun/8055723/client/webrev.01/
>> http://cr.openjdk.java.net/~weijun/8055723/core/webrev.01/
>>
>> There are only 2 now. Everything non-client is in core.
>>
>> Everyone, please do code review quickly because the patch touches too many
>> files and any delay could mean re-merge.
>>
>> *Otávio*: If there is only small change in feedback, tell me to update my
>> own repo and you don't need to generate the big patch again.
>>
>> I see you still include that demo file.
>>
>> Thanks
>> Max
>>
>>
>
Re: URLStreamHandler.getHostAddress() performance
Hi Max,
I don't see any particular reason for this. Maybe it's just a "precaution". It
seems to me it's the only field
of the URL class set directly (without setter) from an outside.
Does it hurt performance a lot? In what cases?
-Pavel
> On 25 Nov 2014, at 12:02, Wang Weijun wrote:
>
> I am benchmarking security manager and notice this
>
> protected synchronized InetAddress getHostAddress(URL u) {
>if (u.hostAddress != null)
>return u.hostAddress;
>
>String host = u.getHost();
>if (host == null || host.equals("")) {
>return null;
>} else {
>try {
>u.hostAddress = InetAddress.getByName(host);
>} catch (UnknownHostException ex) {
>return null;
>} catch (SecurityException se) {
>return null;
>}
>}
>return u.hostAddress;
> }
>
> Is there any reason why the method is synchronized? Why not simply
> "synchronized (u)"?
>
> Thanks
> Max
>
Re: URLStreamHandler.getHostAddress() performance
Max, this change dates back to 99/06/11. And there's not much info left on it.
I suggest changing and running the full test suit.
P.S. I assume the whole synchronization burden in the URL comes from lazy
initialization of hostAddress and lazy computation of the hashCode (...and
access to shared handlers, but that's guarded by different lock and thus of no
interest to us right now). Without the 'hostAddress' the whole object could've
been immutable (given the handler code is ALWAYS executed in the same thread as
constructor, which is hard natural and it's hard to imagine the opposite).
-Pavel
> On 25 Nov 2014, at 12:58, Wang Weijun wrote:
>
>
>> On Nov 25, 2014, at 20:25, Pavel Rappo wrote:
>>
>> Hi Max,
>>
>> I don't see any particular reason for this. Maybe it's just a "precaution".
>> It seems to me it's the only field
>> of the URL class set directly (without setter) from an outside.
>>
>> Does it hurt performance a lot? In what cases?
>
> There is a 7x difference in my experiment on SecureClassLoader.defineClass().
>
> Or, it can be shown using this benchmark:
>
> package org.openjdk.bench;
>
> import org.openjdk.jmh.annotations.*;
>
> import java.io.IOException;
> import java.net.URL;
> import java.util.*;
> import java.util.concurrent.ConcurrentHashMap;
>
> @State(Scope.Benchmark)
> public class Weird {
>final int num = 100;
>final Object[] urls = new Object[num];
>
>public static class CS {
>private final Object url;
>public CS(Object url) { this.url = url; }
>public int hashCode() { return url.hashCode(); }
>public boolean equals(Object o) { return o instanceof CS &&
> url.equals(((CS)o).url); }
>}
>
>private final Map pdcacheC = new ConcurrentHashMap<>();
>
>@Setup
>public void init() throws Exception {
>for (int i=0; i< num; i++) {
>urls[i] = new URL("file:/tmp/"+i);
>}
>}
>
>@State(Scope.Thread)
>public static class ThreadState {
>final Random rand = new Random();
>}
>
>
>@Benchmark
>public void setC(ThreadState state) throws IOException {
>Object cs = new CS(urls[next(state)]);
>pdcacheC.computeIfAbsent(cs, x -> "");
>}
>
>private int next(ThreadState state) {
>return state.rand.nextInt(num);
>}
> }
>
> --Max
>
>
>> -Pavel
>>
>>> On 25 Nov 2014, at 12:02, Wang Weijun wrote:
>>>
>>> I am benchmarking security manager and notice this
>>>
>>> protected synchronized InetAddress getHostAddress(URL u) {
>>> if (u.hostAddress != null)
>>> return u.hostAddress;
>>>
>>> String host = u.getHost();
>>> if (host == null || host.equals("")) {
>>> return null;
>>> } else {
>>> try {
>>> u.hostAddress = InetAddress.getByName(host);
>>> } catch (UnknownHostException ex) {
>>> return null;
>>> } catch (SecurityException se) {
>>> return null;
>>> }
>>> }
>>> return u.hostAddress;
>>> }
>>>
>>> Is there any reason why the method is synchronized? Why not simply
>>> "synchronized (u)"?
>>>
>>> Thanks
>>> Max
>>>
>>
>
Re: URLStreamHandler.getHostAddress() performance
Peter, thanks a lot for the link and such a detailed explanation. I've been working with URL/URLStreamHandler recently to make them "modularization ready" and have noticed some of the problems you were talking about as well. I do think we should make our best effort to fix it. Give me some time and I'll get back to this. Thanks. -Pavel > On 27 Nov 2014, at 20:17, Peter Levart wrote: > > Hi, > > Some time ago I dived into the sinchronization pitfalls of URL / > URLStreamHandler and came up with a possible solution. Here's the thread > (mostly just my comments) and a patch: > > http://mail.openjdk.java.net/pipermail/net-dev/2014-July/008592.html > > Regards, Peter
Re: URLStreamHandler.getHostAddress() performance
> On 25 Nov 2014, at 14:03, Mark Sheppard wrote: > > I think this raises a more fundamental question, as to why the URL > hashCode() and equals() methods delegates to URLStreamHandler > in the first place? rather than performing the processing within the URL > class itself, and synchronizing appropriately within. Mark, I guess it's a part of initial design. Handlers know how to parse each individual URL (URLStreamHandler::parseURL), they know how to split URL into meaningful components. They know what these components mean. Therefore they were probably chosen as the best place calculate hash code and perform equality test. But that's in theory. In practise we have a purely-defined URL::equals and URL::hashCode > If you call equals() and hasCode() concurrently on the same URL instance, > there exists the possibility (slight) that > the hostAddress could be set differently, when it is being set for the first > time, without the synchronization of the getHostAddress() in the > URLStreamHandler. > > Although it may rarely happen the mechanics of InetAddress.getByName() > potentially, lend itself to a different return address > on multiple calls, as it returns the first address from a array of addresses > retrieved - assumption is that the ordering will always be the same. > > regards > Mark Agreed. Correct me if I'm wrong, I believe InetAddress::getByName is inherently inconsistent since a naming service is involved.
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 19:06:04 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>>
>>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>>> 47: * whether some object is the referent of a phantom reference.
>>> 48: * @param the type of the referent
>>
>> Shouldn't there be a space after `@param` ?
>
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
> Shouldn't there be a space after `@param` ?
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
I built the API documentation after this PR has been integrated and the result
was okay. I saw this output in every such case:
Type Parameters:
T - the type of the referent
javadoc is quite robust. However, for some IDEs such missing whitespace seems
significant. Not only do they highlight the `@param` tag, but the type
parameter information is missing from the rendered output.
Although it's not critical, we should fix it; I have filed JDK-8285890.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Fri, 29 Apr 2022 08:45:42 GMT, Pavel Rappo wrote: > Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files. I've created a PR; feel free to review it at your convenience. - PR: https://git.openjdk.java.net/jdk/pull/8410
RFR: 8285890: Fix some @param tags
* Syntactically improves a few cases from 8285676 (https://github.com/openjdk/jdk/pull/8410) * Fixes a few misspelled `@param` tags for elements that, although are not included in the API Documentation, are visible in and processed by IDEs - Commit messages: - Fix misspelled `@param` - Clarify with whitespace tags from 8285676 Changes: https://git.openjdk.java.net/jdk/pull/8465/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8465&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285890 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/8465.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8465/head:pull/8465 PR: https://git.openjdk.java.net/jdk/pull/8465
Integrated: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs This pull request has now been integrated. Changeset: 3eb661bb Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/3eb661bbe7151f3a7e949b6518f57896c2bd4136 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod 8285890: Fix some @param tags Reviewed-by: dfuchs, mullan, darcy, mchung, wetmore - PR: https://git.openjdk.java.net/jdk/pull/8465
