Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v9]
On Tue, 17 Nov 2020 21:21:47 GMT, Roger Riggs wrote: >> Ian Graves has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding test coverage. Tweaking wording in docs. > > test/jdk/java/util/IllegalFormatException/ArgumentIndexException.java line 1: > >> 1: /* > > Typically, using the TestNG framework is preferable for new tests. > In addition to a convenient set of Asserts that check and print expected vs > actual and message > it includes patterns for testing for expected exceptions. Yes, these tests are amenable to TestNG's `assertThrows` method. That might be all you need; we generally aren't too concerned about the exact content and formatting of the exception's message. However, if you want to make additional assertions over the thrown exception, it can be obtained using `expectThrows` instead of `assertThrows`. - PR: https://git.openjdk.java.net/jdk/pull/516
Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v9]
On Tue, 17 Nov 2020 19:58:29 GMT, Ian Graves wrote:
>> The `java.util.Formatter` format specifies support for field widths,
>> argument indexes, or precision lengths of a field that relate to the
>> variadic arguments supplied to the formatter. These numbers are specified by
>> integers, sometimes negative. For argument index, it's specified in the
>> documentation that the highest allowed argument is limited by the largest
>> possible index of an array (ie the largest possible variadic index), but for
>> the other two it's not defined. Moreover, what happens when a number field
>> in a string is too large or too small to be represented by a 32-bit integer
>> type is not defined.
>>
>> This fix adds documentation to specify what error behavior occurs during
>> these cases. Additionally it adds an additional exception type to throw when
>> an invalid argument index is observed.
>>
>> A CSR will be required for this PR.
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding test coverage. Tweaking wording in docs.
test/jdk/java/util/IllegalFormatException/ArgumentIndexException.java line 52:
> 50: }
> 51: }
> 52: throw new RuntimeException("Expected IllegalFormatException for
> zero argument index.");
The wording of errors around exceptions can be misinterpreted. Did an expected
exception occur or not?
If all you saw was the exception without the line of code, would it be
unambiguous?
src/java.base/share/classes/java/util/Formatter.java line 2802:
> 2800: // skip the trailing '$'
> 2801: index = Integer.parseInt(s, start, end - 1, 10);
> 2802: if(index <= 0) {
Add a space after 'if' please.
test/jdk/java/util/IllegalFormatException/ArgumentIndexException.java line 1:
> 1: /*
Typically, using the TestNG framework is preferable for new tests.
In addition to a convenient set of Asserts that check and print expected vs
actual and message
it includes patterns for testing for expected exceptions.
test/jdk/java/util/IllegalFormatException/ArgumentIndexException.java line 98:
> 96: }
> 97:
> 98: }
Add a newline at end-of-file.
src/java.base/share/classes/java/util/IllegalFormatArgumentIndexException.java
line 64:
> 62: public String getMessage() {
> 63: return String.format("Illegal format argument index = %d",
> getIndex());
> 64: }
The exception with a very large negative number isn't going to easy to
recognize.
Can the exception message say (for the Integer.MIN_VALUE) that the index is not
valid index.
Its probably too much to ask have an indication where in the format string the
offending index occurs.
-
PR: https://git.openjdk.java.net/jdk/pull/516
Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v9]
> The `java.util.Formatter` format specifies support for field widths, argument > indexes, or precision lengths of a field that relate to the variadic > arguments supplied to the formatter. These numbers are specified by integers, > sometimes negative. For argument index, it's specified in the documentation > that the highest allowed argument is limited by the largest possible index of > an array (ie the largest possible variadic index), but for the other two it's > not defined. Moreover, what happens when a number field in a string is too > large or too small to be represented by a 32-bit integer type is not defined. > > This fix adds documentation to specify what error behavior occurs during > these cases. Additionally it adds an additional exception type to throw when > an invalid argument index is observed. > > A CSR will be required for this PR. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Adding test coverage. Tweaking wording in docs. - Changes: - all: https://git.openjdk.java.net/jdk/pull/516/files - new: https://git.openjdk.java.net/jdk/pull/516/files/5a0effe1..aaa35af2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=516=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=516=07-08 Stats: 106 lines in 4 files changed: 103 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/516/head:pull/516 PR: https://git.openjdk.java.net/jdk/pull/516
