On Tue, 6 May 2025 10:43:02 GMT, Mikhail Yankelevich <myankelev...@openjdk.org> wrote:
>> Jeremy Wood has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - 8356137: removing sentence fragment >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075200131 >> - 8356137: fixing typo >> - 8356137: adding empty line at bottom of file >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075192144 >> - 8356137: removing layered pass/fail pattern >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075190122 >> - 8356137: removing (c) 2002 >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075186796 > > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 28: > >> 26: * @bug 8356137 >> 27: * @summary This test verifies a non-zero transparent pixel in gifs >> works when >> 28: * the disposal method changes from 2 to 1, and when the > > Nitpick: Do you need the `, and when the` ? 😄 Thanks, this is updated > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 105: > >> 103: // we don't expect this to happen, but if something >> goes >> 104: // wrong nobody else will print our stacktrace for >> us: >> 105: e.printStackTrace(); > > Do you need this print here? Runtime exception should print it out anyway to > the system.error afaik Yes, because this eventually is caught here in GifImageDecoder.java: try { if (!readImage(totalframes == 0, disposal_method, delay)) { return; } } catch (Exception e) { if (verbose) { e.printStackTrace(); } return; } In this specific test file: I never expect an exception to be thrown, but one did come up when I was first drafting this test (because of my own error). It was hard to debug because it was unreported. I would prefer to leave these printStackTrace calls in, in case a developer someday makes a change and needs to see potential errors. (Technically I could try to make verbose `true`, but that's declared as a final variable and I don't want to modify GifImageDecoder.java just for this.) > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 133: > >> 131: } >> 132: } catch(Exception e) { >> 133: e.printStackTrace(); > > Do you need to print out the stack trace here when you are throwing it below? Yes, this is caught in the same place the other one is ( https://github.com/openjdk/jdk/pull/25044#discussion_r2075930867 ) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075932570 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075930867 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075932044
