[cfe-dev] [Patch] Diagnose unused declaration

[Douglas Gregor]

Hello,

On Jan 4, 2009, at 7:59 PM, EdB wrote:
> This time "used" informations are keep "on the side".

Thanks; that's much better.

> There is place  for improvement but I want to be sure that  
> information are at
> the right place before.

A few comments on this iteration:

  static llvm::cl::opt<bool>
+WarnUnusedDeclaration("Wunused_declaration",
+         llvm::cl::desc("Warn for unused declaration"));

I suggest that we use the same command-line arguments as GCC, unless  
there is some specific reason to deviate from them:

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



In Sema::PopDeclContext(), the warning-generation code needs to be  
much more picky about which declarations it complains about. For  
example, I tried this C++ example:

namespace N {
   int x;
}

namespace N {
   int f() {
     return x;
   }
}

and Clang produced 4 warnings: one each for x, f, __builtin_va_list,  
and N:

This option should not produce warnings about any of these, because:

	- x is the definition of a variable that can be referred to by other  
translation units, so even if it isn't used within this translation  
unit, we should not warn about it. Plus, "x" is used later on in the  
program (in a different DeclContext for the namespace N). Note that  
GCC's -Wunused-variable documentation explicitly says that it only  
warns about local variables and non-constant static variables; we  
should do something similar.

	- f is the definition of a function, which could also be referenced  
by other translation units. Note that GCC's -Wunused-function only  
complains about static functions (and, presumably, functions in  
anonymous namespaces) because those aren't visible outside of the  
translation unit.

	- __builtin_va_list wasn't written by the user, so we shouldn't  
complain about it. Besides, do we even want to complain about unused  
typedefs? Especially in C++, there are certainly reasons to write  
typedefs even if they aren't used in the translation unit.

	- N is a namespace; do we ever want to warn about those?

I see that UsedDeclaration grows throughout the processing of the  
translation unit, because nothing is ever removed from the set (even  
when it is no longer visible through name lookup). We'd like to avoid  
that.

Does this warning work properly in C? In Sema::LookupDecl, it looks  
like you need to change

     for (; I != IdResolver.end(); ++I)
       if ((*I)->getIdentifierNamespace() & NS)
	return *I;


to

     for (; I != IdResolver.end(); ++I)
       if ((*I)->getIdentifierNamespace() & NS) {
	UsedDeclaration.insert(*I);
	return *I;
       }

?

Also in Sema::LookupDecl:

-          return MaybeConstructOverloadSet(Context, I, E);
+          Decl *D = MaybeConstructOverloadSet(Context, I, E);
+          UsedDeclaration.insert(static_cast<NamedDecl *>(D));
+          return D;

The result of MaybeConstructOverloadSet could be an  
OverloadedFunctionDecl, which is transient and therefore should not be  
inserted into the UsedDeclaration set (since we'd never know to remove  
it).

Do we need the new code in ActOnEndOfTranslationUnit? It's nearly  
identical to the new code in Sema::PopDeclContext, and translation  
units are DeclContexts. Perhaps this code could be factored out?

Personally, I think that any patch introducing warnings for unused  
variables/parameters/functions/etc. also needs to support GCC's  
__attribute__((unused)) extension to turn off the warning for specific  
declarations. We'll get a lot of complaints from GCC users if we  
produce this warning when they've explicitly asked to suppress it for  
a declaration.

You'll need to include a regression test that works with -verify along  
with your patch, so that we can make sure this functionality doesn't  
break in the future. You'll probably need at least two regression  
tests, one each for C and C++. Try to exercise a bunch of different  
language features in them, e.g., overloading in C++, uses of "extern",  
"inline", etc. Getting this warning right is actually far harder than  
it looks at first blush, but you've got a good start on it.

	- Doug