On Mon, 16 Oct 2023 23:56:24 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> Matthew Donovan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> retained a reference to the RMI server and improved naming > > test/lib/jdk/test/lib/Asserts.java line 588: > >> 586: } >> 587: // fail() throws a RuntimeException so this is unreachable. >> 588: return null; > > Hm, this is unfortunate. Even though this method throws along all code paths, > the compiler doesn't do this analysis. Even though this "never happens" > there's still a question of "but what if?" (I guess it could happen if > somebody in the future were to modify the code above.) I'd suggest > unconditionally throwing an exception here (probably InternalError) to be > more explicit. Returning null will result in an inexplicable NPE if somebody > dereferences the return value. Actually, this is a mistake. If I call `fail()` immediately after the test method executes (because an exception wasn't thrown), the RuntimeException which will promptly be caught in my try/catch and that isn't what we want. I'm moving that invocation to the end which looks and works a lot better. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361958521
