Community
Participate
Working Groups
Build ID: I20090217-0800 Steps To Reproduce: Basically the fix for bug#264991 suppressed the emission of unused constructor message for unused constructors of private classes. These diagnostics were introduced by the fix for bug#201912. These need to be reintroduced in the appropriate manner. Test case: Look at org.eclipse.jdt.core.tests.compiler.regression.ProblemConstructorTest.test004() We should warn about the constructor of X.M#M(int) being unused. We don't. More information:
Created attachment 126014 [details] Proposed patch + tests Kent, can you please review this patch and help take it to commit. Thanks.
In ConstructorDeclaration.analyseCode(), you're finding the super constructor by asking the scope. MethodBinding superCtor = this.scope.getConstructor(superClass, Binding.NO_PARAMETERS, this.constructorCall); if (superCtor == null || !superCtor.isValidBinding()) break checkUnused; Why not just ask the superclass directly? if (superClass.getExactConstructor(Binding.NO_PARAMETERS) == null) break checkUnused;
Ok - so I now see why - we need to check visibility. That can still be done explicitly vs. the scope call which has side affects. I have another question, if a private type has only 1 constructor - why would we ever complain that it is unused ? Does it make sense that the user remove the type's only constructor ?
Created attachment 126152 [details] Patch with review comments incorporated
(In reply to comment #3) > Ok - so I now see why - we need to check visibility. That can still be done > explicitly vs. the scope call which has side affects. I don't think this call has any adverse side effects (I picked up that usage from ExplicitConstructorCall.resolve), but I agree it is needlessly heavyweight for the current needs. The patch posted now addresses this concern. > I have another question, if a private type has only 1 constructor - why would > we ever complain that it is unused ? Does it make sense that the user remove > the type's only constructor ? As long as an element is not explicitly used and (cannot be used in future in unknown ways (by virtue of being private)) it should be ok to warn I think. The litmus test should really be whether each warning is "actionable on an independant basis" and whether the action of deleting the seemingly unused item causes any ill effects (such as program build failure or run time behavior difference) Am I missing something ??
We call scope methods to resolve explicit references in the AST. When you're checking to see if an inherited method exists, we usually just go to the bindings themselves since we do care to report problems, record references, etc. "Am I missing something ??" No - I was just wondering since some users define a 'default' constructor for no reason, so warning that it is unused seems unneccessary. BTW - The comments are backwards for ProblemConstructorTest test008 & test009
Fix and test released for 3.5M6
(In reply to comment #6) > BTW - The comments are backwards for ProblemConstructorTest test008 & test009 Thanks Kent!
Verified for 3.5M6 using I20090310-0100.
