RFR: 8360037: Refactor ImageReader in preparation for Valhalla support [v15]
David Beaumont
duke at openjdk.org
Mon Aug 11 17:10:19 UTC 2025
On Mon, 11 Aug 2025 15:09:56 GMT, Alan Bateman <alanb at openjdk.org> wrote:
>> I mean without that, as a reader of the code who doesn't know details, I'd definitely be questioning why a closeable object (ImageReader) is being obtained and used, but not closed. I probably even added it because I'd had to dig around the code to understand that it was actually correct to not close it here.
>>
>> Perhaps the docs for SystemImage (currently `Holder class for the ImageReader.` could be improved to make the lifecycle clear and benefit all callers of `reader()` ?)
>
> Maybe, or the singleton returned by SystemImage.reader() is non-closeable. My only comment for now is tha the "should not be called by callers." is confusing here and would be better to remove. We do do this in the next change in the area if you want.
The comment is there because that line is:
a) right above a try-catch block and
b) comes with a "click here to put this into a try-with-resources block" lint action in an IDE
So, at a glance, it looks like it's a simple mistake that it's not a resource in the following try-block.
It just feels tempting for a future maintainer to mistakenly "fix it".
That's the "job" of the comment, to catch when someone is about to do that.
I reworded slightly though.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2267466083
More information about the core-libs-dev
mailing list