Skip to content

Conversation

@elharo
Copy link
Contributor

@elharo elharo commented Dec 19, 2025

No description provided.

@elharo elharo marked this pull request as ready for review December 19, 2025 13:35
@elharo elharo requested a review from rmannibucau December 19, 2025 13:35
@rmannibucau
Copy link
Contributor

rmannibucau commented Dec 19, 2025

@elharo hmm, why is it cleaner ? Difference is not much except your flavor adds bytecode for a case which will never happen so basically you add dead code from my window, any real case worth a unit test and this change?

@elharo
Copy link
Contributor Author

elharo commented Dec 19, 2025

  1. It removes a formal variable and keeps everything within the try block. This is how try blocks were designed.
  2. If it turns out that the IOException is not actually impossible, then it's signaled instead of being swallowed so the bug can be addressed.

@rmannibucau
Copy link
Contributor

  1. well this is discussable even if I agree on the intent, but the code pattern you do use is also wrong for try blocks cause it misses the flush (in terms of "cleaness", luckily this does nothing there but if you assume that you can also assume there will never be any IOException) which is a well know issue of the return within the try so looks worse than before to me
  2. not a fan of coding for an hypothesis, there is literally not a single line throwing that exception in the related class so it is impossible and other exceptions will blow up as expected

so tempted to think this PR is not cleaner so really +-0 since it will literally just add dead code and not change the behavior

@elharo
Copy link
Contributor Author

elharo commented Dec 19, 2025

try-with-resources handles the flush. That's what this block was originally trying to do with the ignored variable.

@rmannibucau
Copy link
Contributor

try-with-resources handles the flush.

Please run

class Foo {
    @Test
    void run() throws IOException {
        try (final var writer = new StringWriter() {
            @Override
            public void flush() {
                throw new IllegalStateException("this will not happen");
            }
        }) {
            writer.write("test");
        }
    }
}

That's what this block was originally trying to do with the ignored variable.

No, the current code works cause we call toString after close which does handle flush, no more your version which doesn't work then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants