On Sat, Apr 19, 2014 at 4:22 AM, Nuno Lopes <[email protected]> wrote:
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).
I think the 'is it completely defined' heuristic should be able to return
true for the above class, because we've reached a point where:
* every public and protected member is defined,
* every friend is defined, and
* every *used* private member is defined
Ideally, the heuristic shouldn't depend on the order in which member
definitions are given, but that may be too expensive to track.
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.
Modeling the call graph within Clang in order to track this would
certainly
be an interesting approach, if the cost is low enough (dealing with
non-function members add a wrinkle, but don't seem to fundamentally defeat
this). I can think of a few other warnings that would also benefit from
Clang having some notion of a call graph (we have a "function always
recurses infinitely" warning, but it doesn't cope with mutual recursion,
for instance).
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?
I was thinking: each time you see the definition of a class member, check
if all relevant members of the class have been defined, and if so, emit
your warning immediately. (Maybe keep a count of the number of relevant
members that don't have definitions yet, and process the class when the
count reaches 0, to avoid scanning the class more than once.)
Would this play better with modules?
Yes, it should.
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.
