[cfe-commits] [PATCH] Unused function warning

[Douglas Gregor]

On Jan 28, 2010, at 5:02 PM, Tanya Lattner wrote:

> Doug,
> 
> Thanks so much for the feedback. I've attached an updated patch, but also a couple questions about your review below:
> 
> On Jan 26, 2010, at 8:05 PM, Douglas Gregor wrote:
> 
>> 
>> +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).
>> 
> 
> Sorry, bit new at this. For static functions declared but not defined, doesn't that fall under "missing-prototypes" instead? 

GCC's manual claims that it is under -Wunused-function, here:

	http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Warning-Options.html#Warning-Options

> Right now I put static functions that are definitions onto a list in ActOnFunctionDeclarator. I could change this to be all static functions if you think that isn't covered by the other warning.

I'm sure it's part of the -Wunused-function warning, but I don't feel strongly that we need this part of the warning. It certainly doesn't have to be in the first commit of -Wunused-function.

> For the C++ functions in anonymous namespace, I assume I just check "isInAnonymousNamespace" and thats good? So I would check for that OR static.

Yes, that'll work, although there's an easier way: just check for any function with internal linkage using NamedDecl::getLinkage(). That will cover static functions and functions in anonymous namespaces, along with any wonky way we can end up with a function not visible outside of the translation unit.

>> 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.
>> 
> 
> Well, this "false" warning is popping up (I added it to the test case). I'm guessing there isn't a way to prevent this?

It's possible, but I've had a change of heart on this. Why did the user bother defining the function 'f0' if the definition was never used? It seems legitimate to warn here.

>> I wonder if this warning should have a Fix-It that removes the function entirely :)
> 
> Yes, but its possible someone may not want it removed? :)


A Fix-It hint here is overkill, especially given the sizeof(f0()) case above, where the function name is uttered but the definition isn't used.

A few detailed 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,8 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-function -verify %s
+
+void foo() {}
+static void f2() {} 
+static void f1() {f2();} // expected-warning{{unused}}
+
+static int f0() { return 17; }
+int x = sizeof(f0());

It would be good to also check a few cases with multiple declarations of the function, e.g.,

	static void f3();
	extern void f3() { } // this is static, should complain
	inline static void f4();
 	void f4() { } // inline, should not complain


+  
+  // If there were any unused static functions, deserialize them and add to
+  // Seam's list of unused static functions.

Typo "Seam"

+  
+  // Keep track of static, non-inlined function definitions that
+  // have not been used. We will warn later.
+  if (!NewFD->isInvalidDecl() && IsFunctionDefinition 
+      && !isInline && SC == FunctionDecl::Static
+      && !NewFD->isUsed())
+    UnusedStaticFuncs.push_back(NewFD);
+  

As I mentioned above, I suggest using NewFD->getLinkage() == NamedDecl::InternalLinkage.
Also, I suggest using NewFD->isInlined() rather than isInline, because the call will also consider previous declarations.

+   
+    // Remove used function from our list of static unused functions to prevent
+    // the unused function warning from triggering.
+    if (Function->getStorageClass() == FunctionDecl::Static) {
+      std::vector<FunctionDecl*>::iterator I = std::find(UnusedStaticFuncs.begin(),
+                                                         UnusedStaticFuncs.end(),
+                                                         Function);
+      if (I != UnusedStaticFuncs.end())
+        UnusedStaticFuncs.erase(I);
+    }

This is linear in the number of static function definitions. I don't know if that's a problem in practice, but I have an alternative suggestion: don't bother to remove anything from UnusedStaticFuncs here. Instead, at the beginning of Sema::ActOnEndOfTranslationUnit(), go through the list once to remove all of the static function definitions that are marked "used" (or for which any of their redeclarations was used).

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100129/087f065f/attachment.html>