Thanks for your review! Some comments/questions inline:
Please find in attach a second version of the patch, which reuses the
cache of defined records properly.
With this patch, -Wunused-member-function now flags unused private
methods
whenever the class is fully defined.
This patch does not attempt to fix false positives that were being
triggered before in classes in anonymous namespaces. I'll fix those
afterwards.
OK to commit?
No, it's not OK to delete the early pruning of UnusedFileScopedDecls.
Without that, we'll load in all unused declarations within a module for
every compilation that includes the module.
Ah, my bad, sorry.
I'm really outdated on "recent" clang developments. I totally missed the
LazyVector thing.
The approach taken by warning and the existing unused private field
warning
interact badly with modules in general -- they force us to deserialize
most
private members within every imported module, just in case the class
became
completely-defined within this compilation, because their approach is "for
each maybe-unused thing, check if it's unused, then diagnose". Instead of
the current approach, we could go for "for each class completed in this
compilation, check for unused private members".
Well, that approach may not allow us to be more aggressive to push the
warnings a little beyond.
For example, we don't give any warning on the following example:
class foo {
int x;
void f();
public:
void g() {}
};
This class is not completely defined, since we don't have the body for
'f()'. However, we know that 'f()' is not called from any public method
(nor transitively by any of their callees).
Therefore 'f()' is dead and so is field 'x'.
This pattern actually appears in LLVM code. Someone may have removed the
body of a method, but forgot to remove the declaration, which is then
inhibiting warnings about unused private fields.
I can even imagine more contrived examples:
class c {
void g(...);
void f(...) { g(...); }
public:
void bar() {}
}
c::g() { f(...); }
Although 'f()' and 'g()' are used, they are not reachable from within the
public call graph, and therefore are dead.
I did not attempt to detect the above cases, but I was hopping to target
them next. I'm unsure how to implement the trigger base approach you
suggest. Should we record classes that have all public+protected methods
defined and then iterate over those only? Would this play better with
modules?
Thanks,
Nuno
----- Original Message ----- From: Nuno Lopes
Sent: Sunday, March 23, 2014 11:14 PM
Subject: Improving -Wunused-member-function
Hi,
I would like to improve -Wunused-member-function to detect unused
private
methods, similarly to how -Wunused-private-fields works.
I think clang should be able to flag a method if 1) it is unused, 2) all
the
methods of the class are defined in the TU, and 3) any of the following
conditions holds:
- The method is private.
- The method is protected and the class is final.
- The method is public and the class is in an anonymous namespace.
I have a simple implementation in attach that can handle the first case
(private methods) only.
I'm not very happy with it, though. In particular I would like to move
the
logic somewhere else, so that we can reuse it from Codegen. And right
now
I'm not caching things properly. Any suggestions to where this code
belongs? Should it go directly to Decl? (but that would imply adding a
few
fields for cache purposes).
Any comments and suggestions are welcomed!
Thanks,
Nuno
P.S.: I run the attached patch over the LLVM codebase and I already
fixed
a
bunch of cases it detected (but left many still). So big code bases will
certainly benefit from this analysis. Moreover, removing unused decls
triggered more -Wunused-private-fields warnings.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
