RFR: JDK-8283075: Bad `IllegalArgumentException` message when calling `ClassDesc.arrayType(int)` which results in a rank > 255
Improving the exception messages for out-of-supported-range array types. I'll update copyrights before pushing. - Commit messages: - Appease jcheck - Update @bug line of test - JDK-8283075: Bad `IllegalArgumentException` message when calling `ClassDesc.arrayType(int)` which results in a rank > 255 Changes: https://git.openjdk.java.net/jdk/pull/7812/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7812&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283075 Stats: 29 lines in 2 files changed: 24 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7812.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7812/head:pull/7812 PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255
On Mon, 14 Mar 2022 19:56:17 GMT, Joe Darcy wrote: > Improving the exception messages for out-of-supported-range array types. > > I'll update copyrights before pushing. src/java.base/share/classes/java/lang/constant/ClassDesc.java line 186: > 184: if (rank <= 0 || netRank > > ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS) > 185: throw new IllegalArgumentException("rank: " + netRank + > 186:" exceeds maximum > supported dimension of " + if the `rank < 0` it won't be exceeding the maximum supported dimensions so this message only applies to the second condition - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
> Improving the exception messages for out-of-supported-range array types. > > I'll update copyrights before pushing. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7812/files - new: https://git.openjdk.java.net/jdk/pull/7812/files/51388988..1723d710 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7812&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7812&range=00-01 Stats: 14 lines in 1 file changed: 9 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7812.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7812/head:pull/7812 PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Mon, 14 Mar 2022 21:27:29 GMT, Joe Darcy wrote: >> Improving the exception messages for out-of-supported-range array types. >> >> I'll update copyrights before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. lgtm - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Mon, 14 Mar 2022 21:27:29 GMT, Joe Darcy wrote: >> Improving the exception messages for out-of-supported-range array types. >> >> I'll update copyrights before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Mon, 14 Mar 2022 21:27:29 GMT, Joe Darcy wrote: >> Improving the exception messages for out-of-supported-range array types. >> >> I'll update copyrights before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Changes requested by plevart (Reviewer). src/java.base/share/classes/java/lang/constant/ClassDesc.java line 179: > 177: int netRank; > 178: if (rank <= 0) { > 179: throw new IllegalArgumentException("rank " + rank + "is not > a positive value"); There's a space missing in the IAE message after the rank number. For example, if rank == 0, the message would be: "rank 0is not positive value" ... - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Tue, 15 Mar 2022 07:50:29 GMT, Peter Levart wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to review feedback. > > src/java.base/share/classes/java/lang/constant/ClassDesc.java line 179: > >> 177: int netRank; >> 178: if (rank <= 0) { >> 179: throw new IllegalArgumentException("rank " + rank + "is not >> a positive value"); > > There's a space missing in the IAE message after the rank number. For > example, if rank == 0, the message would be: "rank 0is not positive value" ... Suggestion: throw new IllegalArgumentException("rank " + rank + " is not a positive value"); - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Mon, 14 Mar 2022 21:27:29 GMT, Joe Darcy wrote: >> Improving the exception messages for out-of-supported-range array types. >> >> I'll update copyrights before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. src/java.base/share/classes/java/lang/constant/ClassDesc.java line 185: > 183: netRank = Math.addExact(currentDepth, rank); > 184: if (netRank > ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS) { > 185: throw new IllegalArgumentException("rank: " + netRank + This doesn’t need to use [Math.addExact](currentDepth, rank), as currentDepth will always be in [0, 255], and rank here is confined to [1, 231-1], so it’s possible to check for netRank < 0 and use [Integer.toUnsignedString](netRank) (or [Integer.toUnsignedLong](netRank) and let [StringConcatFactory] deal with it): Suggestion: netRank = currentDepth + rank; if (netRank > ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS || netRank < 0) { throw new IllegalArgumentException("rank: " + Integer.toUnsignedLong(netRank) + [Integer.toUnsignedLong]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Integer.html#toUnsignedLong(int) [Integer.toUnsignedString]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Integer.html#toUnsignedString(int) [Math.addExact]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Math.html#addExact(int,int) [StringConcatFactory]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/StringConcatFactory.html - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v3]
> Improving the exception messages for out-of-supported-range array types. > > I'll update copyrights before pushing. Joe Darcy 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 six additional commits since the last revision: - Update copyright and fix typo. - Merge branch 'master' into JDK-8283075 - Respond to review feedback. - Appease jcheck - Update @bug line of test - JDK-8283075: Bad `IllegalArgumentException` message when calling `ClassDesc.arrayType(int)` which results in a rank > 255 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7812/files - new: https://git.openjdk.java.net/jdk/pull/7812/files/1723d710..514a57a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7812&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7812&range=01-02 Stats: 1594 lines in 67 files changed: 1085 ins; 268 del; 241 mod Patch: https://git.openjdk.java.net/jdk/pull/7812.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7812/head:pull/7812 PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Tue, 15 Mar 2022 13:50:03 GMT, ExE Boss wrote: >> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 179: >> >>> 177: int netRank; >>> 178: if (rank <= 0) { >>> 179: throw new IllegalArgumentException("rank " + rank + "is >>> not a positive value"); >> >> There's a space missing in the IAE message after the rank number. For >> example, if rank == 0, the message would be: "rank 0is not positive value" >> ... > > Suggestion: > > throw new IllegalArgumentException("rank " + rank + " is not a > positive value"); Good catch; will fix before pushing. - PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Tue, 15 Mar 2022 13:49:34 GMT, ExE Boss wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to review feedback. > > src/java.base/share/classes/java/lang/constant/ClassDesc.java line 185: > >> 183: netRank = Math.addExact(currentDepth, rank); >> 184: if (netRank > ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS) >> { >> 185: throw new IllegalArgumentException("rank: " + netRank + > > This doesn’t need to use [Math.addExact](currentDepth, > rank), as currentDepth will always > be in [0, 255], and rank here is confined to [1, > 231-1], so it’s possible to check for netRank > < 0 and use > [Integer.toUnsignedString](netRank) (or > [Integer.toUnsignedLong](netRank) and let > [StringConcatFactory] deal with it): > > Suggestion: > > netRank = currentDepth + rank; > if (netRank > ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS || > netRank < 0) { > throw new IllegalArgumentException("rank: " + > Integer.toUnsignedLong(netRank) + > > > [Integer.toUnsignedLong]: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Integer.html#toUnsignedLong(int) > [Integer.toUnsignedString]: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Integer.html#toUnsignedString(int) > [Math.addExact]: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Math.html#addExact(int,int) > [StringConcatFactory]: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/StringConcatFactory.html Hmm. In this context, I think using addExact is clearer regarding the intention of the check. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7812