RFR: 8360037: Refactor ImageReader in preparation for Valhalla support [v12]

[David Beaumont]

On Fri, 1 Aug 2025 12:56:53 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Hmm, fair. The protected constructor only exists because of ExplodedImage (and I had hoped to be able to get rid of that completely with this work, but sadly couldn't). I added a clear warning to not create other subtypes. I could move this to an abstract method, but honestly I don't think it fixes anything since "completeness" is a concept that only makes sense internally for the implementation in this class (it's package access, so cannot be seen elsewhere). Making it abstract would mean that it needs to be more visible, which I dislike.
>
> In addition to what Roger noted, the default implementation of this `isCompleted()` method and the default implementation of `getLocation()` method a few lines below seem to contradict each other. `isCompleted()` by default returns `true` implying a resource node type but `getLocation()` by default throws an exception, implying a non-resource node type.

"is completed" is a non-directory type, "getLocation() failing" is a non-resource type. The abstract class is consistently saying "I'm not a thing of /that/ type" (because it isn't and these are the only answers it can give).
I don't think there's *any* risk or issue here. The abstract class is internal, clearly documented and overloaded exactly once by a trusted collaborator. Do you really think this is a "risk"? And if you do, please give your suggestion as to what would be better. Otherwise nothing is actionable here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2247987436