Community
Participate
Working Groups
I'm writing some experimental detectors for FindBugs and used Eclipse JDT code to test them. During my testing I've found the following code: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/codegen/BranchLabel.java#n159 public int forwardReferenceCount() { if (this.delegate != null) this.delegate.forwardReferenceCount(); return this.forwardReferenceCount; } public int[] forwardReferences() { if (this.delegate != null) this.delegate.forwardReferences(); return this.forwardReferences; } My new detector marks the if lines as redundant code. Could you please check whether this is intended? It seems that the code should be written like this: public int forwardReferenceCount() { if (this.delegate != null) _return_ this.delegate.forwardReferenceCount(); return this.forwardReferenceCount; } public int[] forwardReferences() { if (this.delegate != null) _return_ this.delegate.forwardReferences(); return this.forwardReferences; } Thank you in advance.
On the one hand, yes, the code definitely looks suspicious. Thanks for letting us know! Unfortunately, the team member who wrote this code 8 years ago is no longer aboard, so it's difficult to speak about *intention*. We'll check what is the desired behaviour, though. OTOH, in order to signal it as *redundant*, you'd have to assert that all implementations (incl. all sub-classes) of forwardReferenceCound() are pure functions, i.e., don't produce side effects.
Thank you for your response. Yes, sure, my detector looks for every implementation in subclasses accessible for analysis. Otherwise there would be too many false-positives. Of course it may be wrong if the method is intended to have side-effects in derived projects that were not supplied to the analysis. My detector is still in early development stage, but hopefully will become something useful.
If that is all the dubious code your tool found in JDT/Core I would say, your tool is dubious, there should be much more suspect code in JDT :) Banter aside, I agree that method looks suspicious. We will take a closer look in the coming days. (In reply to Tagir Valeev from comment #2) > the analysis. My detector is still in early development stage, but hopefully > will become something useful. Good luck, do send us any more diagnostics you uncover. TIA.
Srikanth Sankaran, well there are two more similar places found by my detector if you're interested in: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/parser/RecoveredImport.java#n45 public ImportReference updatedImportReference(){ return this.importReference; } public void updateParseTree(){ updatedImportReference(); } http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/parser/RecoveredAnnotation.java#n216 public Annotation updatedAnnotationReference() { return this.annotation; } ... public void updateParseTree() { updatedAnnotationReference(); } The body of updateParseTree() methods has no effect. Again, there are no overridden side-effect updated*Reference methods, but probably it was planned to extend them later...
One more detection after I've implemented some improvements regarding arrays in my detector: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/org.eclipse.jdt.core/search/org/eclipse/jdt/core/search/SearchPattern.java#n2077 if (!method.isBinary()) { // prefix with a '*' as the full qualification could be bigger (because of an import) CharOperation.concat(IIndexConstants.ONE_STAR, returnQualification); } CharOperation.concat has no side effects, so it seems that result should be assigned to something. See CharOperation.java for detail: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/org.eclipse.jdt.core/compiler/org/eclipse/jdt/core/compiler/CharOperation.java#n907
(In reply to Tagir Valeev from comment #5) > One more detection after I've implemented some improvements regarding arrays > in my detector: Thanks, now I agree your detector is up to snuff and withdraw my remarks made in jest in comment#3 :)
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
These valuable hints should not go to waste. Looks like low-hanging fruit. Anyone?
(In reply to Stephan Herrmann from comment #8) > These valuable hints should not go to waste. > > Looks like low-hanging fruit. > > Anyone? Looks like both the issue and solution is already provided. Only question will be about a testcase. Do we have that already too?
Bulk move of unassigned bugs to 4.15
Bulk move to 4.16
Bulk move of unassigned bugs to 4.17
