[cfe-commits] [PATCH] Unused function warning

Tanya Lattner lattner at apple.com
Thu Jan 28 17:02:23 PST 2010


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? 

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.

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

> +  
> +  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 :(

Makes perfect sense. I have made these changes.

> 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?

> 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? :)


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100128/5ac5e1b3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unusedFuncWarning.patch
Type: application/octet-stream
Size: 7662 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100128/5ac5e1b3/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100128/5ac5e1b3/attachment-0001.html>

More information about the cfe-commits mailing list