[cfe-commits] Clang change to detect and warn about unused private members

Jordan Rose jordan_rose at apple.com
Fri Jun 1 16:54:24 PDT 2012

First off, what do people think about turning this on by default, or at least making it part of -Wunused? Daniel's being conservative by leaving it off, but you know what people say about off-by-default warnings...


As for the patch…sorry, one last problem.

+    // Mark as fully defined for now to stop recursion in case of mutual
+    // friends.
+    FullyDefined.insert(RD);

I thought about this some more, and it seems like this will cause incorrect warnings:

class B;
class A {
  int x;
  friend class B;

class B {
  int y;
  friend class A;

When looking through A's decls (because of A::x), we'll come across "friend class B". We'll look through B's decls and find "friend class A". A has already been marked complete, so we mark B complete. Then we go back to A and find "doSomethingWithX()", and A is now (correctly and finally) marked incomplete. When we get to B::y, though, we're in trouble, because B has already been marked as complete via A!

The usual thing to do here is add an "in progress" set, but I'm still not sure exactly how to use that for warnings. I think the best thing to do (and possibly the most correct thing to do) is to simply give up when we see mutual friends declarations -- that is, tentatively mark classes as not fully defined instead of fully defined. That gives us a conservative set of warnings, even if we miss a few valid cases.

> Finally, even though there's not a big hit traversing the fields at the end, we probably still shouldn't pay for this if the diagnostic is off. Can you check for that before inserting into UnusedPrivateFields, or even doing all the tests?
> I am now checking whether the diagnostic is enabled before inserting into the set and before doing the checks at the end of the translation unit. Checking before removing from the (then empty) set probably does not give any benefit. I am not sure whether the check on the initializer arguments is expensive (especially the call to HasSideEffects). What do you think?

Let's leave it as is. Looking up a diagnostic isn't /that/ cheap, and most initializers are something simple like a VarDecl, a constant, or a FunctionCall. 

I think once you've fixed the friend problem you can go ahead and commit. Thanks for working on this!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120601/144e73ee/attachment.html>

More information about the cfe-commits mailing list