[cfe-commits] [PATCH] Unused function warning

[Douglas Gregor]

On Jan 26, 2010, at 2:32 PM, Tanya Lattner wrote:

> I've attached a patch to implement warning of unused functions (includes a test case).  Please let me know what you think.


I like this warning, thanks for working on it. A couple comments:

Index: test/Sema/warn-unused-function.c
===================================================================
--- test/Sema/warn-unused-function.c	(revision 0)
+++ test/Sema/warn-unused-function.c	(revision 0)
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-function -verify %s
+
+void foo() {}
+
+static void f1() {} // expected-warning{{unused}}

'twould be good to verify that we don't get warnings for a static function that *is* used. 

+def warn_unused_function : Warning<"unused function %0">,
+  InGroup<UnusedFunction>, DefaultIgnore;

GCC's -Wunused-function has a bit more behavior than is in your patch. Obviously, you don't have to implement it all, but please leave FIXMEs for cases that aren't handled (static functions declared but not defined, C++ functions in anonymous namespaces).

+  
+  for (DeclContext::decl_iterator
+       D = Context.getTranslationUnitDecl()->decls_begin(),
+       DEnd = Context.getTranslationUnitDecl()->decls_end();
+       D != DEnd;
+       ++D) {
+    // Check for unused functions.
+    if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*D)) {
+      if (!FD->isUsed() && !FD->hasAttr<UnusedAttr>()
+          && (FD->getStorageClass() == FunctionDecl::Static)) {
+        Diag(FD->getLocation(), diag::warn_unused_function) << FD->getDeclName();
+      }
+    }
+  }

This is going to be expensive, because it's walking the entire translation unit. When precompiled headers are involved, it gets *really* expensive, because it will end up deserializing every top-level declaration to perform this walk.

Instead, I suggest that ActOnFunctionDef recognize when we're defining a static function (that has not yet been used) and put it into a list of such functions. Whenever a static function is marked used for the first time, we remove it from that list. At the end of the translation unit, just walk that list and complain about everything on it. The most annoying part about this is that the list will need to be saved to the PCH file, just like tentative definitions are :(

There are two more cases to think about. First, we shouldn't warn about inline static functions. Second, we should figure out what using one of these static functions as an unevaluated operand should silence the warning. For example:

  static int f0() { return 17; }
  int x = sizeof(f0());

The "used" bit on f0 won't get set, so I'm guessing we'll end up warning about "f0" being unused. The list-of-unused-static-functions approach I mentioned above could avoid this problem, if we don't want the warning there. I don't really have a strong opinion on the matter.

I wonder if this warning should have a Fix-It that removes the function entirely :)

	- Doug