Improving -Wunused-member-function

[Richard Smith]

On Sat, Apr 19, 2014 at 4:22 AM, Nuno Lopes <nunoplopes at sapo.pt> 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.
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140421/8d363bda/attachment.html>