On Thu, 21 Dec 2023 21:51:10 GMT, Eamonn McManus <emcma...@openjdk.org> wrote:
> Multiplying with `*` never produces `ArithmeticException`, so the catch in
> the existing code is never triggered. `Math.multiplyExact` does produce
> `ArithmeticException` if the multiplication overflows. So we can use that,
> and rethrow `IllegalArgumentException` as the specification says.
>
> There is a small compatibility risk, in that code may have been relying on
> the previous silent overflow, and will now get an exception. But an exception
> is surely better than the nonsense results that overflow produces.
>
> Thanks to Kurt Kluever for the test cases.
Please update the copyright years in both the files.
test/jdk/java/sql/testng/test/sql/TimestampTests.java line 649:
> 647: // The latest Instant that can be converted to a Timestamp.
> 648: Instant instant1 = Instant.ofEpochSecond(Long.MAX_VALUE / 1000,
> 999_999_999);
> 649: assertEquals(instant1, Timestamp.from(instant1).toInstant());
The 1st arg to `assertEquals()` should be the actual value, the 2nd should be
the expected result. I guess you expect `instant1` to be the expected result.
(TestNG and JUnit have opposite conventions...)
test/jdk/java/sql/testng/test/sql/TimestampTests.java line 658:
> 656: } catch (IllegalArgumentException expected) {
> 657: }
> 658:
TestNG supports a better way for expected exceptions, as an attribute of the
@Test annotation. Maybe it can be used here for the expected failing cases?
-------------
PR Review: https://git.openjdk.org/jdk/pull/17181#pullrequestreview-1794185027
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1434867044
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1434867239
