RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible

[Raffaello Giulietti]

On Thu, 21 Dec 2023 21:51:10 GMT, Eamonn McManus <emcmanus at 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