<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Apr 19, 2014 at 4:22 AM, Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for your review!  Some comments/questions inline:<div class=""><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please find in attach a second version of the patch, which reuses the<br>
cache of defined records properly.<br>
With this patch, -Wunused-member-function now flags unused private methods<br>
whenever the class is fully defined.<br>
This patch does not attempt to fix false positives that were being<br>
triggered before in classes in anonymous namespaces.  I'll fix those<br>
afterwards.<br>
<br>
OK to commit?<br>
<br>
</blockquote>
<br>
No, it's not OK to delete the early pruning of UnusedFileScopedDecls.<br>
Without that, we'll load in all unused declarations within a module for<br>
every compilation that includes the module.<br>
</blockquote>
<br></div>
Ah, my bad, sorry.<br>
I'm really outdated on "recent" clang developments. I totally missed the LazyVector thing.<div class=""><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The approach taken by warning and the existing unused private field warning<br>
interact badly with modules in general -- they force us to deserialize most<br>
private members within every imported module, just in case the class became<br>
completely-defined within this compilation, because their approach is "for<br>
each maybe-unused thing, check if it's unused, then diagnose". Instead of<br>
the current approach, we could go for "for each class completed in this<br>
compilation, check for unused private members".<br>
</blockquote>
<br></div>
Well, that approach may not allow us to be more aggressive to push the warnings a little beyond.<br>
For example, we don't give any warning on the following example:<br>
<br>
class foo {<br>
 int x;<br>
 void f();<br>
<br>
public:<br>
 void g() {}<br>
};<br>
<br>
<br>
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).<br></blockquote>
<div><br></div><div>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:</div><div> * every public and protected member is defined,</div>
<div> * every friend is defined, and</div><div> * every *used* private member is defined</div><div>Ideally, the heuristic shouldn't depend on the order in which member definitions are given, but that may be too expensive to track.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Therefore 'f()' is dead and so is field 'x'.<br>
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.<br>
<br>
I can even imagine more contrived examples:<br>
<br>
class c {<br>
 void g(...);<br>
 void f(...) { g(...); }<br>
<br>
public:<br>
 void bar() {}<br>
}<br>
<br>
c::g() { f(...); }<br>
<br>
<br>
Although 'f()' and 'g()' are used, they are not reachable from within the public call graph, and therefore are dead.<br></blockquote><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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?</blockquote>
<div><br></div><div>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.)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Would this play better with modules?</blockquote><div><br></div><div>Yes, it should.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
Thanks,<br>
Nuno<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
----- Original Message ----- From: Nuno Lopes<br>
Sent: Sunday, March 23, 2014 11:14 PM<br>
Subject: Improving -Wunused-member-function<br>
<br>
<br>
 Hi,<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would like to improve -Wunused-member-function to detect unused private<br>
methods, similarly to how -Wunused-private-fields works.<br>
I think clang should be able to flag a method if 1) it is unused, 2) all<br>
the<br>
methods of the class are defined in the TU, and 3) any of the following<br>
conditions holds:<br>
- The method is private.<br>
- The method is protected and the class is final.<br>
- The method is public and the class is in an anonymous namespace.<br>
<br>
I have a simple implementation in attach that can handle the first case<br>
(private methods) only.<br>
I'm not very happy with it, though. In particular I would like to move the<br>
logic somewhere else, so that we can reuse it from Codegen. And right now<br>
I'm not caching things properly.  Any suggestions to where this code<br>
belongs?  Should it go directly to Decl? (but that would imply adding a<br>
few<br>
fields for cache purposes).<br>
<br>
Any comments and suggestions are welcomed!<br>
<br>
Thanks,<br>
Nuno<br>
<br>
P.S.: I run the attached patch over the LLVM codebase and I already fixed<br>
a<br>
bunch of cases it detected (but left many still). So big code bases will<br>
certainly benefit from this analysis. Moreover, removing unused decls<br>
triggered more -Wunused-private-fields  warnings. <br>
</blockquote></blockquote></blockquote>
<br>
</div></div></blockquote></div><br></div></div>