Improving -Wunused-member-function

[Nuno Lopes]

Thanks for the reply!
I'll work on a new patch next week or so.

Thanks,
Nuno

----- Original Message -----
> 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.