On Fri, 1 Aug 2025 12:56:53 GMT, Jaikiran Pai <j...@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
