On Mon, 28 Feb 2022 14:03:15 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Explorative refactoring performed while looking into multiple `@inheritDoc` >> issues. The easiest way to review it is to, probably, go commit by commit; >> they are quite focused and commented. Not only the branch as a whole, but >> all the constituent commits should pass tests and leave JDK API >> Documentation unchanged. > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 28 commits: > > - Merge branch 'master' into 8280713 > - Fix outdated code > > Uses a set instead of list for quick method search. In a future commit > we'll try to figure out why `found` are not unique. > - Fix outdated code > > Fix outdated inline comments and names. Both the assertion and the > `contains` method will be removed in a future commit. > - Fix ImplementedMethods.overridingMethodFound > > Removes code from > VisibleMemberTable.ImplementedMethods.overridingMethodFound; the removed code > is dead for the reasons similar to those in the previous commit. This commit > makes overridingMethodFound degrade to linear search, which could be > substituted with hashing in a later commit. > - Remove ImplementedMethods::removeOverriddenMethod > > First of all, this commit has been tested and the before and after JDK API > Documentation compared. > > `removeOverriddenMethod` does not seem to do any visible work and only > confuses the reader. Here are some problems, all of which has been confirmed > while debugging. Hopefully, the bellow bullet points in conjunction with the > source code will help you see these problems. > > Outside: > > 1. `removeOverriddenMethod` is private and is only used in one place: > VisibleMemberTable.java:1015 > 2. `VisibleMemberTable` passes `removeOverriddenMethod` with a direct > interface method, an interface method that is either defined or overridden by > an interface, but not passively inherited. > > Inside: > > 1. `overriddenClass` is either a class that first defined a method that > the passed `method` overrides, or `null` if there's no such class. > 2. `overriddenClass` is searched for recursively, using only supertypes. > 3. Regardless of any interfaces it might extend, an interface's supertype > is `java.lang.Object`; see, for example, > `javax.lang.model.element.TypeElement#getSuperclass`. > 4. `method` belongs to an interface; see "Outside" (2). > 5. Given 1-4, the only class `overriddenClass` can be is > `java.lang.Object`. This can happen, for example, if `method` is an > interface-override of one of the methods defined in `java.lang.Object`. > Typically those are `equals`, `hashCode` or `toString`. > 7. `isSubclassOf(a, b)` checks if `a` is a subtype of `b`. > 8. Since `a` is `java.lang.Object`, condition `b == a || isSubclassOf(a, > b)` can be `true` only if `b` is also `java.lang.Object`. `java.lang.Object` > is not a subclass of anything but itself. > 9. `b` can never be a class, let alone `java.lang.Object`. This is because > `b` is an immediately enclosing type of an interface method (by construction > of `methlist`), i.e. `b` is an interface. > 10. So the above check always fails, which means that `methlist.remove(i)` > is never executed. > - Simplify VisibleMemberTable.ImplementedMethods > > Also adds a TODO to remind us to investigate an important issue later; > either in this or a follow-up PR. > - Fix confusing doc > - Merge branch 'master' into 8280713 > - Refine overriddenType(ExecutableElement) > > Makes it clear that the returned TypeMirror is DeclaredType which > represents a class and never an interface. > - Refactor how superinterfaces are collected > > Improves readability of Utils.getAllInterfaces and friends. > - ... and 18 more: > https://git.openjdk.java.net/jdk/compare/efd3967b...41a00ce9 Good cleanup, as a step along the way to a better documented/specified form, even if parts of it were a little obscure. ------------- Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7233
