Improving -Wunused-member-function

[Nuno Lopes]

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.