Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v9]
On Mon, 11 Dec 2023 20:09:09 GMT, Sergey Tsypanov wrote: >> src/java.base/share/classes/java/io/ByteArrayInputStream.java line 212: >> >>> 210: // 'tmpbuf' is null if and only if 'out' is trusted >>> 211: byte[] tmpbuf; >>> 212: if (IOStreams.isTrusted(out)) >> >> IMHO we should use `IOStreams.isTrusted(out)` in BAIS, too. > > BAIS - ByteArrayInputStream? It's already there Sorry, it seems I missed that. đź‘Ť - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1424972112
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v12]
On Tue, 12 Dec 2023 20:18:15 GMT, Sergey Tsypanov wrote:
>> It looks like we can skip copying of `byte[]` in
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in
>> `java.io`.
>>
>> See comment by @vlsi in
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8320971: Whitespaces
src/java.base/share/classes/com/sun/io/IOStreams.java line 53:
> 51: public static boolean isTrusted(OutputStream os) {
> 52: var clazz = os.getClass();
> 53: return clazz == ByteArrayOutputStream.class
I wonder what the committers think: Do we need a unit test for each trusted
class to proof that they actually behave accordingly to the rules of trust
listed in the Javadocs?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1424981445
Re: [jdk22] RFR: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
On Mon, 11 Dec 2023 10:19:52 GMT, Adam Sotona wrote: > 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec > contains a copy-paste error Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/5#pullrequestreview-1779043371
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
> It looks like we can skip copying of `byte[]` in > `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in > `java.io`. > > See comment by @vlsi in > https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: 8320971: Fix JavaDoc - Changes: - all: https://git.openjdk.org/jdk/pull/16879/files - new: https://git.openjdk.org/jdk/pull/16879/files/a3ba43fd..c6696e27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=11-12 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879 PR: https://git.openjdk.org/jdk/pull/16879
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v12]
On Tue, 12 Dec 2023 20:18:15 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8320971: Whitespaces Thank you for this contribution. If I were a committer I would be voting +1. :-) - Marked as reviewed by mk...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/16879#pullrequestreview-1779047929
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 08:15:21 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8320971: Fix JavaDoc test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 52: > 50: byte[] tmp = new byte[len]; > 51: RND.nextBytes(tmp); > 52: System.arraycopy(tmp, 0, b, off, len); It is sad there's no `Random#nextBytes(byte[] buf, int offs, int len)` - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425023714
[jdk22] Integrated: 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error
On Mon, 11 Dec 2023 10:19:52 GMT, Adam Sotona wrote: > 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec > contains a copy-paste error This pull request has now been integrated. Changeset: a55e18ba Author:Adam Sotona URL: https://git.openjdk.org/jdk22/commit/a55e18baf093aa53aae954e3c9650770d703266e Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8321641: ClassFile ModuleAttribute.ModuleAttributeBuilder::moduleVersion spec contains a copy-paste error Reviewed-by: alanb Backport-of: 3c6459e1de9e75898a1b32a95acf684050fbe1af - PR: https://git.openjdk.org/jdk22/pull/5
Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]
On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona wrote:
>> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is
>> corrupted so the parser attempts to read beyond the classfile bounds.
>> General contract is that only `IllegalArgumentException` or its subclasses
>> is expected when parser fails.
>> This patch wraps `IndexOutOfBoundsExceptions` thrown from all
>> `ClassReaderImpl.buffer` manipulations into an
>> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
>> Relevant tests are added.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains three commits:
>
> - Merge branch 'master' into JDK-8320360-bounds
>
># Conflicts:
># test/jdk/jdk/classfile/LimitsTest.java
> - added bug # to the test
> - 8320360: ClassFile.parse: Some defect class files cause unexpected
> exceptions to be thrown
May I ask for review of this fix?
Thank you.
-
PR Comment: https://git.openjdk.org/jdk/pull/16762#issuecomment-1853511673
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v14]
> It looks like we can skip copying of `byte[]` in > `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in > `java.io`. > > See comment by @vlsi in > https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: 8320971: Add more tests - Changes: - all: https://git.openjdk.org/jdk/pull/16879/files - new: https://git.openjdk.org/jdk/pull/16879/files/c6696e27..42147ce8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=12-13 Stats: 40 lines in 1 file changed: 36 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879 PR: https://git.openjdk.org/jdk/pull/16879
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v12]
On Wed, 13 Dec 2023 08:04:59 GMT, Markus KARG wrote:
>> Sergey Tsypanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8320971: Whitespaces
>
> src/java.base/share/classes/com/sun/io/IOStreams.java line 53:
>
>> 51: public static boolean isTrusted(OutputStream os) {
>> 52: var clazz = os.getClass();
>> 53: return clazz == ByteArrayOutputStream.class
>
> I wonder what the committers think: Do we need a unit test for each trusted
> class to proof that they actually behave accordingly to the rules of trust
> listed in the Javadocs?
Added tests for all involved OutputStreams
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425050812
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 08:41:37 GMT, Vladimir Sitnikov wrote: >> Sergey Tsypanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8320971: Fix JavaDoc > > test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 52: > >> 50: byte[] tmp = new byte[len]; >> 51: RND.nextBytes(tmp); >> 52: System.arraycopy(tmp, 0, b, off, len); > > It is sad there's no `Random#nextBytes(byte[] buf, int offs, int len)` Feel free to open a PR. :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425052398
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 08:15:21 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8320971: Fix JavaDoc src/java.base/share/classes/com/sun/io/IOStreams.java line 26: > 24: */ > 25: > 26: package com.sun.io; The starting point for this PR is getting agreement to relax the checks in BufferedInputStream.transferTo so ideally the changes would be limited to that one method initially. If you really want a supporting utility class then a non-public class like java.io.TransferSupport or something like that would be easier to discuss. We don't need to adding a new package com.sun.io for this. I think drop j.u.zip.CheckedOutputStream from the initial discussion as it it "too far away" to be in the mix at this point. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425055930
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 09:09:29 GMT, Alan Bateman wrote: >> Sergey Tsypanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8320971: Fix JavaDoc > > src/java.base/share/classes/com/sun/io/IOStreams.java line 26: > >> 24: */ >> 25: >> 26: package com.sun.io; > > The starting point for this PR is getting agreement to relax the checks in > BufferedInputStream.transferTo so ideally the changes would be limited to > that one method initially. If you really want a supporting utility class then > a non-public class like java.io.TransferSupport or something like that would > be easier to discuss. We don't need to adding a new package com.sun.io for > this. > > I think drop j.u.zip.CheckedOutputStream from the initial discussion as it > it "too far away" to be in the mix at this point. So what is the target package for this utility class? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425104701
Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v12]
On Tue, 12 Dec 2023 17:33:12 GMT, Srinivas Vamsi Parasa wrote: >> @vamsi-parasa You said: >>> Made sure that OpenJDK builds without errors using both GCC 7.5 and GCC 6.4. >> >> but now we have https://bugs.openjdk.org/browse/JDK-8321688. Did you >> introduce any changes after you tested with GCC 7.5? It seems strange to me >> that the code simultaneously both works and not works with gcc 7.5. > > Hi Magnus (@magicus), did a fresh pull of the OpenJDK and was able to build > it successfully (without any errors) using GCC 7.5.0 on Ubuntu Linux machine. > (I am on vacation till Jan7th, 2024. Our team will look into this issue) New information in JDK-8321688 says it is only happening on slowdebug. This is probably why it was missed. - PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1425106097
Re: ZipEntry
On 12/12/2023 20:17, Alan Snyder wrote: ZipEntry is a public class and I am aware that it is used outside the JDK. Presumably that is not a problem. I’m wondering why the class stores the external file attributes field but does not provide public accessors for it. I would find it useful to have access to this field. I would rather not have to switch to using a third party library just for this reason. I understand that the interpretation of the external file attributes is not standardized, but the field itself is defined in the standard cited by the package info page, so I don’t see the harm in making it accessible without interpretation. It might be useful if you would expand a bit on what your usage is. Is it file permissions, symbolic links, or something else? -Alan
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 09:47:50 GMT, Sergey Tsypanov wrote: > So what is the target package for this utility class? If you really want a utility class then a non-public class in java.io is okay. However, I think the starting point for this change is not the utility class, it's about deciding whether to relax the check on the target or do nothing. The concerns with BIS and BAIS do overlap but they are not identical. More specifically, for BAIS, the underlying byte[] can outlive the BAIS object. For BIS, transferTo may return a positive value more than once because the underlying stream is growing. These are the details that need to be worked through. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425121886
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
> Since JDK-8311906, if CompactStrings is not enabled, index is not considered > when calling extractCodepoints from StringUTF16.toBytes(). Because of that > the last elements of the source codepoints array are stripped from the > resulting UTF16 string, which fires in other places (e.g. during RegEx > processing). > > The fix replaces len in extractCodepoints parameters with end that is index + > len. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17057/files - new: https://git.openjdk.org/jdk/pull/17057/files/759068e8..cf622fae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17057&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17057&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17057.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17057/head:pull/17057 PR: https://git.openjdk.org/jdk/pull/17057
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Wed, 13 Dec 2023 11:39:19 GMT, Aleksei Voitylov wrote: >> Since JDK-8311906, if CompactStrings is not enabled, index is not considered >> when calling extractCodepoints from StringUTF16.toBytes(). Because of that >> the last elements of the source codepoints array are stripped from the >> resulting UTF16 string, which fires in other places (e.g. during RegEx >> processing). >> >> The fix replaces len in extractCodepoints parameters with end that is index >> + len. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > review comments 1. Yes, I'm aware of -XX:-CompactStrings being used in production in server deployments (no, I didn't collect the reasons for that). 2. Yes, it was discovered as part of release testing. Also related to our support efforts for ARM32, but not just that. If you think it's worth it, I can go through some tests and add some more -XX:-CompactStrings mode to java.lang.String tests here and there, lightly. It's not the first time we hit this issue. - PR Comment: https://git.openjdk.org/jdk/pull/17057#issuecomment-1853758819
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v2]
On Tue, 12 Dec 2023 19:11:43 GMT, Roger Riggs wrote: >> Aleksei Voitylov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review comments > > test/jdk/java/lang/String/Chars.java line 50: > >> 48: testChars(cc, ccExp); >> 49: testCharsSubrange(cc, ccExp); >> 50: testIntsSubrange(ccExp); > > Please also add the same call after line 74 to apply the test to the > surrogates case. > (As suggested by @rgiulietti in https://github.com/openjdk/jdk/pull/17066) Just pulled the update from that branch. - PR Review Comment: https://git.openjdk.org/jdk/pull/17057#discussion_r1425241518
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 07:08:53 GMT, David Holmes wrote:
>> Improve the test by being more lenient to related code cache exhaustion
>> errors. The important thing is that we don't terminate with a fatal error,
>> which the new code now checks for explicitly. The check for that is based on
>> what is done by
>> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>>
>> The existing `UpcallTestHelper.Output` class that was previously used to
>> assert on stdout/stderr contents did not have the capability to look for
>> patterns in the output. So, I've taken the opportunity to replace it with
>> the more canonical `OutputAnalyzer` which comes from the test library.
>>
>> Finally, I've also added back the test for downcall stub allocation failure
>> which was removed as part of the initial patch because it was too
>> inconsistent [1]. With the new approach, it should pass reliably as well.
>>
>> Testing: `jdk_foreign` suite (which contains all the affected tests)
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
>
> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>
>> 49: runInNewProcess(UpcallRunner.class, true,
>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>> 50: .shouldNotHaveExitValue(0)
>> 51: .shouldNotHaveFatalError();
>
> Just curious what non-zero exit value is actually expected here and below?
Any non-zero exit value is acceptable. The intent here is to check that the
process didn't complete normally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1425357636
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Wed, 13 Dec 2023 11:39:19 GMT, Aleksei Voitylov wrote: >> Since JDK-8311906, if CompactStrings is not enabled, index is not considered >> when calling extractCodepoints from StringUTF16.toBytes(). Because of that >> the last elements of the source codepoints array are stripped from the >> resulting UTF16 string, which fires in other places (e.g. during RegEx >> processing). >> >> The fix replaces len in extractCodepoints parameters with end that is index >> + len. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > review comments Looks good, thanks for the followup. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17057#pullrequestreview-1779792024
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Wed, 13 Dec 2023 11:39:29 GMT, Aleksei Voitylov wrote: > If you think it's worth it, I can go through some tests and add some more > -XX:-CompactStrings mode to java.lang.String tests here and there, lightly. > It's not the first time we hit this issue. Verifying the coverage of the tests would be useful. I noticed in many tests of the API signatures that include offset and length, the offset is frequently zero. As for -XX:-CompactStrings, it was originally a precaution about backward compatibility. From time to time it comes up to consider simplifying the code to remove it. It would be good to know the use cases where it is needed and the rationale. - PR Comment: https://git.openjdk.org/jdk/pull/17057#issuecomment-1854045141
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Wed, 13 Dec 2023 11:39:19 GMT, Aleksei Voitylov wrote: >> Since JDK-8311906, if CompactStrings is not enabled, index is not considered >> when calling extractCodepoints from StringUTF16.toBytes(). Because of that >> the last elements of the source codepoints array are stripped from the >> resulting UTF16 string, which fires in other places (e.g. during RegEx >> processing). >> >> The fix replaces len in extractCodepoints parameters with end that is index >> + len. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > review comments I'll keep an eye out for the backport to jdk 21 if a review is needed for that. - PR Comment: https://git.openjdk.org/jdk/pull/17057#issuecomment-1854050370
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 10:01:30 GMT, Alan Bateman wrote: >> So what is the target package for this utility class? > >> So what is the target package for this utility class? > > If you really want a utility class then a non-public class in java.io is > okay. However, I think the starting point for this change is not the utility > class, it's about deciding whether to relax the check on the target or do > nothing. The concerns with BIS and BAIS do overlap but they are not > identical. More specifically, for BAIS, the underlying byte[] can outlive the > BAIS object. For BIS, transferTo may return a positive value more than once > because the underlying stream is growing. These are the details that need to > be worked through. @AlanBateman Why not using com.sun as we did it with other internal classes like ChannelInputStream etc.? Regarding your last sentence actually I do not see what "return a positive value more than once" has to do with the actual changed proposed by this PR? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425523315
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 10:01:30 GMT, Alan Bateman wrote: >> So what is the target package for this utility class? > >> So what is the target package for this utility class? > > If you really want a utility class then a non-public class in java.io is > okay. However, I think the starting point for this change is not the utility > class, it's about deciding whether to relax the check on the target or do > nothing. The concerns with BIS and BAIS do overlap but they are not > identical. More specifically, for BAIS, the underlying byte[] can outlive the > BAIS object. For BIS, transferTo may return a positive value more than once > because the underlying stream is growing. These are the details that need to > be worked through. > @AlanBateman Why not using com.sun as we did it with other internal classes > like ChannelInputStream etc.? com.sun.* is the name space we used a long time ago for JDK-specific APIs and some internal classes. A lot more internal classes in sun.*. It doesn't make sense here to add a new package com.sun.io for a single method class. This PR does not need to introduce any new classes at this point. I think this PR needs to focus solely on BIS. > Regarding your last sentence actually I do not see what "return a positive > value more than once" has to do with the actual changed proposed by this PR? On the surface, transferTo reads to EOF so there are no more bytes to read from the underlying stream or byte[]. Adding a "growing file" and it breaks assumptions. So it's an important discussion point in this topic. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r142299
Re: ZipEntry
My immediate interest is symlinks. Alan > On Dec 13, 2023, at 1:55 AM, Alan Bateman wrote: > > On 12/12/2023 20:17, Alan Snyder wrote: >> ZipEntry is a public class and I am aware that it is used outside the JDK. >> Presumably that is not a problem. >> >> I’m wondering why the class stores the external file attributes field but >> does not provide public accessors for it. >> >> I would find it useful to have access to this field. I would rather not have >> to switch to using a third party library just for this reason. >> >> I understand that the interpretation of the external file attributes is not >> standardized, but the field itself is defined in the standard >> cited by the package info page, so I don’t see the harm in making it >> accessible without interpretation. > > It might be useful if you would expand a bit on what your usage is. Is it > file permissions, symbolic links, or something else? > > -Alan
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 15:53:09 GMT, Alan Bateman wrote: > It doesn't make sense here to add a new package com.sun.io for a single > method class. This PR does not need to introduce any new classes at this > point. I think this PR needs to focus solely on BIS. So you actually prefer copy-and-paste and duplicated code? > On the surface, transferTo reads to EOF so there are no more bytes to read > from the underlying stream or byte[]. Adding a "growing file" and it breaks > assumptions. So it's an important discussion point in this topic. I still do not see what this has to do with the *actual* changes proposed by this PR. Can you please provide an example in what situation the *actual* changes proposed by this PR will fail in that case, while the original code will succeed? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425592402
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 16:20:58 GMT, Markus KARG wrote: > > It doesn't make sense here to add a new package com.sun.io for a single > > method class. This PR does not need to introduce any new classes at this > > point. I think this PR needs to focus solely on BIS. > > So you actually prefer copy-and-paste and duplicated code? It's not important at this time, the starting point is to decide if BIS.transferTo should be changed or not. It does shared some concerns with BAIS but the set of concerns for both is not identical. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425625184
Integrated: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote: > Improve the test by being more lenient to related code cache exhaustion > errors. The important thing is that we don't terminate with a fatal error, > which the new code now checks for explicitly. The check for that is based on > what is done by > `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` . > > The existing `UpcallTestHelper.Output` class that was previously used to > assert on stdout/stderr contents did not have the capability to look for > patterns in the output. So, I've taken the opportunity to replace it with the > more canonical `OutputAnalyzer` which comes from the test library. > > Finally, I've also added back the test for downcall stub allocation failure > which was removed as part of the initial patch because it was too > inconsistent [1]. With the new approach, it should pass reliably as well. > > Testing: `jdk_foreign` suite (which contains all the affected tests) > > [1]: > https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64 This pull request has now been integrated. Changeset: 7ece9e90 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/17056
[jdk22] RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
Hi all, This pull request contains a backport of commit [7ece9e90](https://github.com/openjdk/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Jorn Vernee on 13 Dec 2023 and was reviewed by Maurizio Cimadamore. Thanks! - Commit messages: - Backport 7ece9e90c0198f92cdf8d620e346c4a9832724cd Changes: https://git.openjdk.org/jdk22/pull/11/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=11&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321400 Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod Patch: https://git.openjdk.org/jdk22/pull/11.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/11/head:pull/11 PR: https://git.openjdk.org/jdk22/pull/11
Re: RFR: 8320919: Clarify Locale related system properties [v4]
> This is a doc change to clarify what the `Default Locale` is, and how it is > established during the system startup using the system properties. Those > locale-related system properties have existed since the early days of Java, > but have never been publicly documented before. It is also the intention of > this PR to clarify those system properties and how they are overridden. A > corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17065/files - new: https://git.openjdk.org/jdk/pull/17065/files/8baa56a9..60b891a2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17065.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17065/head:pull/17065 PR: https://git.openjdk.org/jdk/pull/17065
Re: RFR: 8320919: Clarify Locale related system properties [v3]
On Mon, 11 Dec 2023 23:20:25 GMT, Naoto Sato wrote: >> This is a doc change to clarify what the `Default Locale` is, and how it is >> established during the system startup using the system properties. Those >> locale-related system properties have existed since the early days of Java, >> but have never been publicly documented before. It is also the intention of >> this PR to clarify those system properties and how they are overridden. A >> corresponding CSR has been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments Thanks, Stuart. Modified the wordings as suggested. - PR Comment: https://git.openjdk.org/jdk/pull/17065#issuecomment-1854460819
RFR: 8318971 : Better Error Handling for Jar Tool Processing of @File
Better Error Handling for Jar Tool Processing of "@File" This is a new PR for this PR since the original developer left the team. See all of the review history at https://github.com/openjdk/jdk/pull/16423. Thank you. - Commit messages: - 8318971 : Better Error Handling for Jar Tool Processing of @File Changes: https://git.openjdk.org/jdk/pull/17088/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17088&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318971 Stats: 56 lines in 2 files changed: 54 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17088.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17088/head:pull/17088 PR: https://git.openjdk.org/jdk/pull/17088
Re: RFR: 8320919: Clarify Locale related system properties [v4]
On Wed, 13 Dec 2023 18:01:55 GMT, Naoto Sato wrote:
>> This is a doc change to clarify what the `Default Locale` is, and how it is
>> established during the system startup using the system properties. Those
>> locale-related system properties have existed since the early days of Java,
>> but have never been publicly documented before. It is also the intention of
>> this PR to clarify those system properties and how they are overridden. A
>> corresponding CSR has been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflects review comments
Marked as reviewed by rriggs (Reviewer).
src/java.base/share/classes/java/util/Locale.java line 305:
> 303: * results in a default Locale with no extensions, while specifying
> 304: * {@code -Duser.language=foobarbaz} results in a default Locale whose
> language is
> 305: * "foobarbaz".)
The parenthesized sentence is so long that its not effective in making it seem
like an option.
The "Typical," is sufficient to indicate it is not part of the spec. Drop the
parens.
src/java.base/share/classes/java/util/Locale.java line 317:
> 315: * If the default Locale is changed with {@link #setDefault(Locale)},
> the corresponding
> 316: * system properties are not altered. It is not recommended that
> applications read
> 317: * these system properties and parse/interpret them as their values may
> be out of date.
"parse/interpret" -> "parse or interpret".
-
PR Review: https://git.openjdk.org/jdk/pull/17065#pullrequestreview-1780257611
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425741255
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425740095
Re: RFR: 8310994: Add JFR event for selection operations [v2]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Change event generation: - selectNow is filtered out - select that times out is always sent - select without timeout uses duration test - rename event to SelectorSelect, field to selectionKeyCount. - Merge branch 'master' into JDK-8310994 - remove trailing whitespace - event logic outside of the lock, selector in try block - remove unused import - fix TestConfigure failure - add event defaults - Merge branch 'master' into JDK-8310994 - minor test cleanup - ... and 2 more: https://git.openjdk.org/jdk/compare/a4905c7c...2f7dafd8 - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/fbb93112..2f7dafd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=00-01 Stats: 73918 lines in 2175 files changed: 42613 ins; 21404 del; 9901 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: JDK-8319626: Override toString() for ZipFile [v8]
On Mon, 11 Dec 2023 05:47:33 GMT, Justin Lu wrote: >> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319982) >> which overrides and provides an implementation of `toString()` in >> _java.util.zip.ZipFile_ (and by extension, _java.util.jar.JarFile_). > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8319626 > - break line > - reflect review: change impl to store field name > - drop additional specification > - drop 2nd paragraph > - return base name, not full path > - shorten wording > - reflect review: use separator, clarify spec > - reflect review: change string value and drop spec > - init I'm seeing the `ZipSourceCache` fail on GHA on `windows-x64`: FAILED ZipSourceCache::testKeySourceMapping 'testKeySourceMapping()' java.nio.file.FileSystemException: 1702471080605-bug8317678.zip: The process cannot access the file because it is being used by another process at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108) at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:273) at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109) at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191) at ZipSourceCache.cleanup(ZipSourceCache.java:65) Build summary: https://github.com/eirbjo/jdk/actions/runs/7194395508 - PR Comment: https://git.openjdk.org/jdk/pull/16643#issuecomment-1854508435
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Fri, 8 Dec 2023 06:30:21 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/event/SelectionEvent.java line 38:
>>
>>> 36: public class SelectionEvent extends Event {
>>> 37:
>>> 38: public int count;
>>
>> It could also be interesting to provide the `timeout` that was given to the
>> selection operation.
>
>> It could also be interesting to provide the `timeout` that was given to the
>> selection operation.
>
> I've tried to work through issues, esp. around selector spinning, and being
> able to distinguish select from selectNow is important for all of them, so
> yes, the timeout is needed or else no emit when the timeout == 0 as that's
> the case you have to filter out when troubleshooting.
I've added filtering of selectNow(), and an event is emitted if there is a
timeout independent of the threshold. The duration should roughly equal the
timout in that case. I added more test cases to cover those two changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425744977
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Thu, 23 Nov 2023 11:18:45 GMT, Erik Gahlin wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/SelectionEvent.java line 43:
>>
>>> 41:
>>> 42: @Label("Selection Count")
>>> 43: @Description("Number of channels selected")
>>
>> I suspect you'll need to rename this event to something like
>> "SelectorSelect" as "Selection" could be anything.
>>
>> We'll to find a better name for the field and the label too. There are two
>> forms of selection operations. One form operates on a selected-key set where
>> the select/selectNow methods returns the number of keys aded to the
>> Selector's ready set. The other form performs an action on each selected
>> key. I'll try to come up a suggestions for the names, I suspect a label
>> "number of channels ready for I/O or added to ready set" would be the most
>> accurate.
>
> It would also be good if the name reflect that it is related to channels so
> it won't clash with other events in the future.
I've made the suggested changes. I changed the "count" field to
"selectorKeyCount" which hopefully seems reasonable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425746321
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:38:09 GMT, Tim Prinzing wrote:
>> Added mirror event with static methods: jdk.internal.event.SelectionEvent
>> that provides the duration of select calls and the count of how many keys
>> are available.
>>
>> Emit the event from SelectorImpl::lockAndDoSelect
>>
>> Test at jdk.jfr.event.io.TestSelectionEvents
>
> Tim Prinzing has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 12 additional
> commits since the last revision:
>
> - Change event generation:
>
>- selectNow is filtered out
>- select that times out is always sent
>- select without timeout uses duration test
> - rename event to SelectorSelect, field to selectionKeyCount.
> - Merge branch 'master' into JDK-8310994
> - remove trailing whitespace
> - event logic outside of the lock, selector in try block
> - remove unused import
> - fix TestConfigure failure
> - add event defaults
> - Merge branch 'master' into JDK-8310994
> - minor test cleanup
> - ... and 2 more: https://git.openjdk.org/jdk/compare/ddad6141...2f7dafd8
src/java.base/share/classes/jdk/internal/event/SelectorSelectEvent.java line 41:
> 39: public class SelectorSelectEvent extends Event {
> 40:
> 41: public int selectionKeyCount;
I still believe we should record the timeout parameter in the event.
src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 44:
> 42: @Label("SelectionKey Count")
> 43: @Description("Number of channels ready for I/O or added to ready set")
> 44: public int selectionKeyCount;
same here
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425754281
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425755618
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:38:11 GMT, Tim Prinzing wrote: >>> It could also be interesting to provide the `timeout` that was given to the >>> selection operation. >> >> I've tried to work through issues, esp. around selector spinning, and being >> able to distinguish select from selectNow is important for all of them, so >> yes, the timeout is needed or else no emit when the timeout == 0 as that's >> the case you have to filter out when troubleshooting. > > I've added filtering of selectNow(), and an event is emitted if there is a > timeout independent of the threshold. The duration should roughly equal the > timout in that case. I added more test cases to cover those two changes. The select call may also exit early with 0 key selected if it was woken up by a call to Selector::wakeup and this is not necessarily indicative of an issue in the API. We use this facility a lot in the HttpClient as we also use the selector thread as a timer thread. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425760691
Re: RFR: 8320919: Clarify Locale related system properties [v4]
On Wed, 13 Dec 2023 18:33:13 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflects review comments
>
> src/java.base/share/classes/java/util/Locale.java line 317:
>
>> 315: * If the default Locale is changed with {@link #setDefault(Locale)},
>> the corresponding
>> 316: * system properties are not altered. It is not recommended that
>> applications read
>> 317: * these system properties and parse/interpret them as their values may
>> be out of date.
>
> "parse/interpret" -> "parse or interpret".
Thanks, Roger. Modified.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425763234
Re: RFR: 8320919: Clarify Locale related system properties [v5]
> This is a doc change to clarify what the `Default Locale` is, and how it is > established during the system startup using the system properties. Those > locale-related system properties have existed since the early days of Java, > but have never been publicly documented before. It is also the intention of > this PR to clarify those system properties and how they are overridden. A > corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17065/files - new: https://git.openjdk.org/jdk/pull/17065/files/60b891a2..48466817 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17065.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17065/head:pull/17065 PR: https://git.openjdk.org/jdk/pull/17065
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:47:50 GMT, Daniel Fuchs wrote:
>> Tim Prinzing has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Change event generation:
>>
>>- selectNow is filtered out
>>- select that times out is always sent
>>- select without timeout uses duration test
>> - rename event to SelectorSelect, field to selectionKeyCount.
>> - Merge branch 'master' into JDK-8310994
>> - remove trailing whitespace
>> - event logic outside of the lock, selector in try block
>> - remove unused import
>> - fix TestConfigure failure
>> - add event defaults
>> - Merge branch 'master' into JDK-8310994
>> - minor test cleanup
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/f9910028...2f7dafd8
>
> src/java.base/share/classes/jdk/internal/event/SelectorSelectEvent.java line
> 41:
>
>> 39: public class SelectorSelectEvent extends Event {
>> 40:
>> 41: public int selectionKeyCount;
>
> I still believe we should record the timeout parameter in the event.
Good point about the wakeup. I'll add the field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425765145
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v13]
On Wed, 13 Dec 2023 16:46:16 GMT, Alan Bateman wrote: >>> It doesn't make sense here to add a new package com.sun.io for a single >>> method class. This PR does not need to introduce any new classes at this >>> point. I think this PR needs to focus solely on BIS. >> >> So you actually prefer copy-and-paste and duplicated code? >> >>> On the surface, transferTo reads to EOF so there are no more bytes to read >>> from the underlying stream or byte[]. Adding a "growing file" and it breaks >>> assumptions. So it's an important discussion point in this topic. >> >> I still do not see what this has to do with the *actual* changes proposed by >> this PR. Can you please provide an example in what situation the *actual* >> changes proposed by this PR will fail in that case, while the original code >> will succeed? > >> > It doesn't make sense here to add a new package com.sun.io for a single >> > method class. This PR does not need to introduce any new classes at this >> > point. I think this PR needs to focus solely on BIS. >> >> So you actually prefer copy-and-paste and duplicated code? > > It's not important at this time, the starting point is to decide if > BIS.transferTo should be changed or not. It does shared some concerns with > BAIS but the set of concerns for both is not identical. > the underlying byte[] can outlive the BAIS object I guess this is true only for target OutputStreams incorporating another one or OutputStreams deliberately made malicious. For classes enumerated in IOStreams this is not a case, I think. > For BIS, transferTo may return a positive value more than once because the > underlying stream is growing As of this I don't understand how it affects the change. The JavaDoc of InputStream.transferTo() explicitly says that the method ` Reads all bytes from this input stream and writes the bytes to the given output stream` and I guess all the data is read only once - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1425794463
RFR: 8259637: java.io.File.getCanonicalPath() returns different values for same path
Modify the `collapse()` function to remove each instance of ".." when the path is absolute and there is no preceding name. - Commit messages: - 8259637: java.io.File.getCanonicalPath() returns different values for same path Changes: https://git.openjdk.org/jdk/pull/17089/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17089&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8259637 Stats: 44 lines in 2 files changed: 37 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17089.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17089/head:pull/17089 PR: https://git.openjdk.org/jdk/pull/17089
Re: RFR: 8259637: java.io.File.getCanonicalPath() returns different values for same path
On Wed, 13 Dec 2023 19:37:15 GMT, Brian Burkhalter wrote: > Modify the `collapse()` function to remove each instance of ".." when the > path is absolute and there is no preceding name. Without this change the updated test fails as: FAILED GetCanonicalPath::goodPathsUnix '[3] /../../../../../a/../../b/c, /b/c' org.opentest4j.AssertionFailedError: expected: but was: FAILED GetCanonicalPath::goodPathsUnix '[5] /../../../../../a/../../../../b/c, /b/c' org.opentest4j.AssertionFailedError: expected: but was: - PR Comment: https://git.openjdk.org/jdk/pull/17089#issuecomment-1854591185
RFR: 8322027: One XMLStreamException constructor fails to initialize cause
One of the three `XMLStreamException` constructors that takes a `Throwable` fails to pass it to the superclass constructor. This simple patch fixes that omission. - Commit messages: - Propagate cause to superclass constructor in XMLStreamException constructor. Changes: https://git.openjdk.org/jdk/pull/17090/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17090&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322027 Stats: 63 lines in 2 files changed: 62 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17090.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17090/head:pull/17090 PR: https://git.openjdk.org/jdk/pull/17090
RFR: 8320279: Link issues in java.xml module-info.java
Doc-only change: fix incorrect links in module-info.java and StAX factories. - Commit messages: - 8320279: Link issues in java.xml module-info.java Changes: https://git.openjdk.org/jdk/pull/17093/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17093&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320279 Stats: 20 lines in 5 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/17093.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17093/head:pull/17093 PR: https://git.openjdk.org/jdk/pull/17093
Re: RFR: JDK-8319626: Override toString() for ZipFile [v8]
On Wed, 13 Dec 2023 18:35:40 GMT, Eirik Bjorsnos wrote: > I'm seeing the `ZipSourceCache` fail on GHA on `windows-x64`: Hi Eirik, Please let me know if I'm misunderstanding, But this change hasn't been integrated into the master branch yet. Your linked PR does not appear to contain the contents of this change either. So I would expect that GHA error is unrelated to this change. I have re-ran the test from the GHA failure (against this PR's contents) on various platforms and all appear to pass for me. - PR Comment: https://git.openjdk.org/jdk/pull/16643#issuecomment-1854685052
Re: RFR: JDK-8319626: Override toString() for ZipFile [v8]
On Wed, 13 Dec 2023 20:53:17 GMT, Justin Lu wrote: > > I'm seeing the `ZipSourceCache` fail on GHA on `windows-x64`: > > Hi Eirik, > > Please let me know if I'm misunderstanding, I claim `TooManyOpenTabsException` :-) Sorry, this comment was for #16115 - PR Comment: https://git.openjdk.org/jdk/pull/16643#issuecomment-1854696711
Re: RFR: 8320279: Link issues in java.xml module-info.java
On Wed, 13 Dec 2023 20:54:16 GMT, Joe Wang wrote: > Doc-only change: fix incorrect links in module-info.java and StAX factories. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17093#pullrequestreview-1780486241
Re: RFR: 8320279: Link issues in java.xml module-info.java
On Wed, 13 Dec 2023 20:54:16 GMT, Joe Wang wrote: > Doc-only change: fix incorrect links in module-info.java and StAX factories. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17093#pullrequestreview-1780488559
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v11]
On Mon, 23 Oct 2023 16:12:45 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Update jmh test comments I noticed the ZipSourceCache test fail on GHA on windows-x64: FAILED ZipSourceCache::testKeySourceMapping 'testKeySourceMapping()' java.nio.file.FileSystemException: 1702471080605-bug8317678.zip: The process cannot access the file because it is being used by another process at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108) at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:273) at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109) at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191) at ZipSourceCache.cleanup(ZipSourceCache.java:65) On a re-run of the jobs, the test ran green again on the same source. So this seems timing related. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1854702162
Re: RFR: 8320279: Link issues in java.xml module-info.java
On Wed, 13 Dec 2023 20:54:16 GMT, Joe Wang wrote: > Doc-only change: fix incorrect links in module-info.java and StAX factories. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17093#pullrequestreview-1780499321
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 20:06:03 GMT, Archie Cobbs wrote: > One of the three `XMLStreamException` constructors that takes a `Throwable` > fails to pass it to the superclass constructor. > > This simple patch fixes that omission. > > It's worth considering if there is any code out there that is working around > this problem already by invoking `initCause()` manually. If so, that code > would start throwing an `IllegalStateException` after this change. > > So a more conservative fix would be to just add another constructor taking > the same arguments in a different order. But then again that's not much > better than just saying "always use initCause() with the broken constructor", > i.e., don't change anything. > > Hmm. Looks good to me. Thanks. test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamExceptionTest/ExceptionCauseTest.java line 38: > 36: * @test > 37: * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest > 38: * @run testng/othervm -DrunSecMngr=true -Djava.security.manager=allow > stream.XMLStreamExceptionTest.ExceptionCauseTest We no longer add this run in new tests as the Security Manager has been deprecated (https://openjdk.org/jeps/411). - Marked as reviewed by joehw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17090#pullrequestreview-1780507743 PR Review Comment: https://git.openjdk.org/jdk/pull/17090#discussion_r1425895822
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 21:19:34 GMT, Joe Wang wrote: >> One of the three `XMLStreamException` constructors that takes a `Throwable` >> fails to pass it to the superclass constructor. >> >> This simple patch fixes that omission. >> >> It's worth considering if there is any code out there that is working around >> this problem already by invoking `initCause()` manually. If so, that code >> would start throwing an `IllegalStateException` after this change. >> >> So a more conservative fix would be to just add another constructor taking >> the same arguments in a different order. But then again that's not much >> better than just saying "always use initCause() with the broken >> constructor", i.e., don't change anything. >> >> Hmm. > > test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamExceptionTest/ExceptionCauseTest.java > line 38: > >> 36: * @test >> 37: * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest >> 38: * @run testng/othervm -DrunSecMngr=true -Djava.security.manager=allow >> stream.XMLStreamExceptionTest.ExceptionCauseTest > > We no longer add this run in new tests as the Security Manager has been > deprecated (https://openjdk.org/jeps/411). Well, the SM is still technically supported so until we remove it, I would leave the run with SM in - PR Review Comment: https://git.openjdk.org/jdk/pull/17090#discussion_r1425898345
Re: RFR: 8320919: Clarify Locale related system properties [v5]
On Wed, 13 Dec 2023 05:19:25 GMT, Stuart Marks wrote:
>> The `-D` command-line option is not a part of the Java SE specification but
>> an allowed behavior, so it may not be a normative description here.
>
> Right, I suggested putting that in parentheses. Historically we haven't been
> very formal about distinguishing between normative (Java SE) specifications
> and informative text that talks about implementations. In this case I felt
> that enclosing the text in parentheses and also adding hedging words
> ("typically") made it clear that this text isn't normative.
FWIW, for the FFM API, we describe the `--enable-native-access` command line
flag in a separate `@implNote`:
https://github.com/openjdk/jdk/blob/cf948548c390c42ca63525d41a9d63ff31349c3a/src/java.base/share/classes/java/lang/foreign/package-info.java#L164-L171
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425900844
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 20:06:03 GMT, Archie Cobbs wrote: > One of the three `XMLStreamException` constructors that takes a `Throwable` > fails to pass it to the superclass constructor. > > This simple patch fixes that omission. > > It's worth considering if there is any code out there that is working around > this problem already by invoking `initCause()` manually. If so, that code > would start throwing an `IllegalStateException` after this change. > > So a more conservative fix would be to just add another constructor taking > the same arguments in a different order. But then again that's not much > better than just saying "always use initCause() with the broken constructor", > i.e., don't change anything. > > Hmm. After filing this PR, I had some second thoughts and added them to the summary but by then it was too late for the `core-libs-dev` email, so I'll repeat here in case anyone has some comments: > It's worth considering if there is any code out there that is working around > this problem already by invoking `initCause()` manually. If so, that code > would start throwing an `IllegalStateException` after this change. > > So a more conservative fix would be to just add another constructor taking > the same arguments in a different order. But then again that's not much > better than just saying "always use `initCause()` with the broken > constructor", i.e., don't change anything. > > Hmm. - PR Comment: https://git.openjdk.org/jdk/pull/17090#issuecomment-1854723627
RFR: 8322018: Test java/lang/String/CompactString/MaxSizeUTF16String.java fails with -Xcomp
The test java/lang/String/CompactString/MaxSizeUTF16String.java fails when run with -Xcomp. Both the java implementation and the intrinsic for StringUTF16.toBytes() allocate memory for a copy of the string. The java implementation of `toBytes()` throws an exception with a message in terms of length of the string. The intrinsic uses a generic message when allocating a byte array that is too large for the implementation. Test should accept either message on the OOME exception, the message is an implementation detail and should reflect the cause of the error and not be confused with a general out of java heap message. - Commit messages: - Handle the case of insufficient heap distinctly from the wrong message - 8322018: Test java/lang/String/CompactString/MaxSizeUTF16String.java fails with -Xcomp Changes: https://git.openjdk.org/jdk/pull/17095/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17095&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322018 Stats: 19 lines in 1 file changed: 14 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17095.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17095/head:pull/17095 PR: https://git.openjdk.org/jdk/pull/17095
Re: RFR: 8320919: Clarify Locale related system properties [v5]
On Wed, 13 Dec 2023 21:25:13 GMT, Jorn Vernee wrote:
>> Right, I suggested putting that in parentheses. Historically we haven't been
>> very formal about distinguishing between normative (Java SE) specifications
>> and informative text that talks about implementations. In this case I felt
>> that enclosing the text in parentheses and also adding hedging words
>> ("typically") made it clear that this text isn't normative.
>
> FWIW, for the FFM API, we describe the `--enable-native-access` command line
> flag in a separate `@implNote`:
> https://github.com/openjdk/jdk/blob/cf948548c390c42ca63525d41a9d63ff31349c3a/src/java.base/share/classes/java/lang/foreign/package-info.java#L164-L171
Thanks for the suggestion, Jorn. I tried embedding `@implNote` within the
ordered list, but looks like it is not allowed. It would be nice if we could
use it inside a list, but since it is about overriding the property values, I
would rather not have the description apart from that list item.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425921350
Re: RFR: 8320575: generic type information lost on mandated parameters [v7]
On Tue, 12 Dec 2023 22:21:29 GMT, Joe Darcy wrote: > As the core reflection code will encounter record classes compiled before and > after the javac code generation change, if the old behavior can be triggered > in javac using `--release $OLD`/`--source $OLD`, that would be helpful to > include as part of the testing. I don't think that the old behavior can be triggered using --release $OLD as fix for https://bugs.openjdk.org/browse/JDK-8292275 was applicable to all targets / sources. But I think I can create a .jcod file with a compact constructor of a record class for which the parameters are not mandated and check that the generic information is correctly retrieved. - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-1854778693
Re: RFR: 8310994: Add JFR event for selection operations [v3]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: add select timeout field to the event - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/2f7dafd8..13262293 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=01-02 Stats: 21 lines in 4 files changed: 12 ins; 3 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
RFR: 8322041: JDK 22 RDP1 L10n resource files update
Translation drop for JDK22 RDP1 - Commit messages: - translated files Changes: https://git.openjdk.org/jdk/pull/17096/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17096&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322041 Stats: 290 lines in 36 files changed: 166 ins; 15 del; 109 mod Patch: https://git.openjdk.org/jdk/pull/17096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17096/head:pull/17096 PR: https://git.openjdk.org/jdk/pull/17096
Introduce constructor for PriorityQueue with existing collection and custom comparator
Hi all, I have raised the following PR, could someone please help me to get it merged? https://github.com/openjdk/jdk/pull/17045 *More details:* *This commit addresses the current limitation in the `PriorityQueue` implementation, which lacks a constructor to efficiently create a priority queue with a custom comparator and an existing collection. In order to create such a queue, we currently need to initialize a new queue with custom comparator, and after that populate the queue using `addAll()` method, which in the background calls `add()` method (which takes `O(logn)` time) for each element of the collection (`n` times). This is resulting in an overall time complexity of `O(nlogn)`. ```PriorityQueue pq = new PriorityQueue<>(customComparator);pq.addAll(existingCollection);```The pull request introduces a new constructor to streamline this process and reduce the time complexity to `O(n)`. If you create the queue above using the new constructor, the contents of the collection will be copied (which takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) will be called once (another `O(n)` time). Overall the operation will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time.```PriorityQueue pq = new PriorityQueue<>(existingCollection, customComparator);```* Best regards, Valeh Hajiyev
RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect
Small document correction on a return description. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/17098/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17098&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321958 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17098/head:pull/17098 PR: https://git.openjdk.org/jdk/pull/17098
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 @alexeysemenyukoracle please review jpackage part - PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1854847266
Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
Sorry, there's a necessary process that a PR must follow. You seem to have signed OCA already. For the rest, see this resource: https://openjdk.org/guide/. In particular, this part: https://openjdk.org/guide/#contributing-to-an-openjdk-project. -Pavel > On 13 Dec 2023, at 23:09, Valeh Hajiyev wrote: > > Hi all, > > I have raised the following PR, could someone please help me to get it merged? > > https://github.com/openjdk/jdk/pull/17045 > > More details: > > This commit addresses the current limitation in the `PriorityQueue` > implementation, which lacks a constructor to efficiently create a priority > queue with a custom comparator and an existing collection. In order to create > such a queue, we currently need to initialize a new queue with custom > comparator, and after that populate the queue using `addAll()` method, which > in the background calls `add()` method (which takes `O(logn)` time) for each > element of the collection (`n` times). This is resulting in an overall time > complexity of `O(nlogn)`. > > ``` > PriorityQueue pq = new PriorityQueue<>(customComparator); > pq.addAll(existingCollection); > ``` > > The pull request introduces a new constructor to streamline this process and > reduce the time complexity to `O(n)`. If you create the queue above using > the new constructor, the contents of the collection will be copied (which > takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) > will be called once (another `O(n)` time). Overall the operation will be > reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > ``` > PriorityQueue pq = new PriorityQueue<>(existingCollection, > customComparator); > ``` > > Best regards, > Valeh Hajiyev >
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 @JoeWang-Java can u review the xml changes? @jonathan-gibbons can u review the compiler changes? - PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1854853944 PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1854854368
Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
Hi Pavel, Thanks for the reply. If I understand correctly, I need this change to be discussed in one of the mailing lists there, so that someone would sponsor me to create a tracking issue in JBS. Do you know which mailing list is the most relevant for me to propose the change? Thanks, Valeh On Thu, Dec 14, 2023 at 12:26 AM Pavel Rappo wrote: > Sorry, there's a necessary process that a PR must follow. You seem to have > signed OCA already. For the rest, see this resource: > https://openjdk.org/guide/. In particular, this part: > https://openjdk.org/guide/#contributing-to-an-openjdk-project. > > -Pavel > > > On 13 Dec 2023, at 23:09, Valeh Hajiyev wrote: > > > > Hi all, > > > > I have raised the following PR, could someone please help me to get it > merged? > > > > https://github.com/openjdk/jdk/pull/17045 > > > > More details: > > > > This commit addresses the current limitation in the `PriorityQueue` > implementation, which lacks a constructor to efficiently create a priority > queue with a custom comparator and an existing collection. In order to > create such a queue, we currently need to initialize a new queue with > custom comparator, and after that populate the queue using `addAll()` > method, which in the background calls `add()` method (which takes `O(logn)` > time) for each element of the collection (`n` times). This is resulting in > an overall time complexity of `O(nlogn)`. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(customComparator); > > pq.addAll(existingCollection); > > ``` > > > > The pull request introduces a new constructor to streamline this process > and reduce the time complexity to `O(n)`. If you create the queue above > using the new constructor, the contents of the collection will be copied > (which takes `O(n)` time) and then later `heapify()` operation (Floyd's > algorithm) will be called once (another `O(n)` time). Overall the operation > will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > customComparator); > > ``` > > > > Best regards, > > Valeh Hajiyev > > > >
Re: RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect
On Wed, 13 Dec 2023 23:10:05 GMT, Naoto Sato wrote: > Small document correction on a return description. Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17098#pullrequestreview-1780672305
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 jpackage part looks fine. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1780705685
Re: RFR: 8320575: generic type information lost on mandated parameters [v8]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/e48c7989..89032d47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=06-07 Stats: 278 lines in 3 files changed: 269 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: RFR: 8320575: generic type information lost on mandated parameters [v9]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: adding a comment to the test - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/89032d47..03ecb887 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=07-08 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: RFR: 8320575: generic type information lost on mandated parameters [v10]
> Reflection is not retrieving generic type information for mandated > parameters. This is a known issue which has been explicitly stated in the API > of some reflection methods. Fix for > [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the > parameters of compact constructors of record classes `mandated` as specified > in the spec. But this implied that users that previous to this change could > retrieve the generic type of parameters of compact constructors now they > can't anymore. The proposed fix is to try to retrieve generic type > information for mandated parameters if available plus changing the spec of > the related reflection methods. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: adding comment to jcod file - Changes: - all: https://git.openjdk.org/jdk/pull/17070/files - new: https://git.openjdk.org/jdk/pull/17070/files/03ecb887..f6e837d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17070&range=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17070/head:pull/17070 PR: https://git.openjdk.org/jdk/pull/17070
Re: RFR: 8320575: generic type information lost on mandated parameters [v9]
On Thu, 14 Dec 2023 03:56:11 GMT, Vicente Romero wrote: >> Reflection is not retrieving generic type information for mandated >> parameters. This is a known issue which has been explicitly stated in the >> API of some reflection methods. Fix for >> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the >> parameters of compact constructors of record classes `mandated` as specified >> in the spec. But this implied that users that previous to this change could >> retrieve the generic type of parameters of compact constructors now they >> can't anymore. The proposed fix is to try to retrieve generic type >> information for mandated parameters if available plus changing the spec of >> the related reflection methods. >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding a comment to the test I have uploaded a few commits addressing the review comments, thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-1855089636
RFR: 8322065: Initial nroff manpage generation for JDK 23
Updated the version to 23-ea and year to 2024. This initial generation also picks up the unpublished changes from: - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & jarsigner) - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK 23 backport) - [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) In addition this includes the updates for - [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags in JDK 23 Thanks - Commit messages: - 8322065: Initial nroff manpage generation for JDK 23 - 8309981: Remove expired flags in JDK 23 Changes: https://git.openjdk.org/jdk/pull/17101/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17101&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322065 Stats: 216 lines in 29 files changed: 47 ins; 61 del; 108 mod Patch: https://git.openjdk.org/jdk/pull/17101.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17101/head:pull/17101 PR: https://git.openjdk.org/jdk/pull/17101
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Fri, 10 Feb 2023 17:38:51 GMT, Brian Burkhalter wrote:
>> `java.io.InputStream::transferTo` could conceivably return a negative value
>> if the count of bytes transferred overflows a `long`. Modify the method to
>> limit the number of bytes transferred to `Long.MAX_VALUE` per invocation.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8297632: Align SequenceInputStream style with other changes in patch
src/java.base/share/classes/java/io/SequenceInputStream.java line 249:
> 247: transferred = Math.addExact(transferred,
> in.transferTo(out));
> 248: } catch (ArithmeticException ignore) {
> 249: return Long.MAX_VALUE;
@bplb , this looks like a bug to me: once `transferred` reaches
`Long.MAX_VALUE` the transfer loop will terminate and the transfer will stop
even in the case there are more streams available in the sequence.
I think the proper code should be `transferred = Long.MAX_VALUE` instead of
`return Long.MAX_VALUE`.
What do you think?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1426217875
Integrated: 8320279: Link issues in java.xml module-info.java
On Wed, 13 Dec 2023 20:54:16 GMT, Joe Wang wrote: > Doc-only change: fix incorrect links in module-info.java and StAX factories. This pull request has now been integrated. Changeset: ddbbd36e Author:Joe Wang URL: https://git.openjdk.org/jdk/commit/ddbbd36e4b064b9e7433f0a55973d72cd6dbc0d3 Stats: 20 lines in 5 files changed: 0 ins; 0 del; 20 mod 8320279: Link issues in java.xml module-info.java Reviewed-by: iris, lancea, naoto - PR: https://git.openjdk.org/jdk/pull/17093
Re: RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect
On Wed, 13 Dec 2023 23:10:05 GMT, Naoto Sato wrote: > Small document correction on a return description. Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17098#pullrequestreview-1781215288
