Community
Participate
Working Groups
Even prior to JDK1.5, we're reporting errors about inherited methods that are unnecessary. For example, with this case: class X extends S { public String foo(Number n) {return null;} // incompatible with T.foo() } abstract class Y extends S {} // complained about T.foo() & S.foo() abstract class T { abstract Object foo(Number n); } abstract class S extends T { abstract String foo(Number n); // REAL problem is this method } The 2 errors reported against X & Y are unnecessary, as the real problem is the return type of S.foo() is not compatible with T.foo(). With the 1.5 verifier, the problem is worse because we needed to keep more inherited methods around to create bridge methods.
Created attachment 65087 [details] Proposed patch
Maxime - please take a look & tell me what you think.
Kent, interrupted by the verification start, just dumping my thoughts so far: - I believe that the test toSkip[i] == -1 on MethodVerifier#600 never yields true, because you only happen to set toSkip[i] (never toSkip[j], that could be forward, hence get the test to yield true); this does not break the code, but could be optimized out; (pls shout if I'm wrong); - I wonder if we could not swap the isInterface test and the inner loop here (not counted what it saves or looses yet; also wondering if we have stats available about a potential ordering of classes/interfaces in the methods array - I mean, do we have some chances that inheriting more often come first, or inherited more often come first): if (length > 1) { for (int i = 0; i < length; i++) { if (toSkip != null && toSkip[i] == -1) continue; // because extends is transitive (anyone which overrides i overrides anyone i overrides and marked it as such) ReferenceBinding declaringClass = methods[i].declaringClass; for (int j = i + 1; j < length; j++) { ReferenceBinding declaringClass2 = methods[j].declaringClass; if (declaringClass.isInterface()) { if (declaringClass2.isInterface()) { if (declaringClass.implementsInterface(declaringClass2, true)) { if (toSkip == null) toSkip = new int[length]; toSkip[j] = -1; } else if (declaringClass2.implementsInterface(declaringClass, true)) { if (toSkip == null) toSkip = new int[length]; toSkip[i] = -1; } } } else if (!declaringClass2.isInterface()) { if (declaringClass2.isSuperclassOf(declaringClass)) { if (toSkip == null) toSkip = new int[length]; toSkip[j] = -1; } else if (declaringClass.isSuperclassOf(declaringClass2)) { if (toSkip == null) toSkip = new int[length]; toSkip[i] = -1; } } } } } - I would suggest that you add a reference to this bug number to at least one of the test cases that illustrate the concern that probed you to open the bug, for the sake of cross referencing, or else that you add a specific regression test.
Released to HEAD for 3.3M7 Updated the loop in findOverriddenInheritedMethods() slightly. This fix makes the one in bug 162073 obsolete
I've ironed out some stats on findOverriddenInheritedMethods#length using a full source workspace. My findings on build I20070501-0010 were the following: Full source workspace stats Length 1 2 3 TOTAL Raw 64565 1627 5 66197 Percent 97,53% 2,46% 0,01% 100,00% I believe this is worth some consideration. Since this bug is fixed, I opened bug 1850376 to that effect. Otherwise, I think the move to cutting down the list of methods to examine upfront is a good idea, and the modifications you made to the loop go even beyond my expectations. Coming back after the battle was over, I did not make a systematic review of the remainder of the patch. I can still do it if you want me to.
(In reply to comment #5) Since bug #1850376 does not exist, bug #185037 looks like the correct one.
(In reply to comment #6) > Since bug #1850376 does not exist, bug #185037 looks like the correct one. Yeah. Typo. Thx for pointing it out.
Verified for 3.3M7 with I20070503-1400.
