Implementing -Wunused-variables

Hi,

So I'm trying to get my feet wet with clang, and I noticed that right now clang doesn't notice when you declare a variable but don't use it. I wrote this simple testcase and put it in

llvm/tools/clang/test/Sema/warning-unused-variables.c

Hello Oscar,

So I'm trying to get my feet wet with clang, and I noticed that right
now clang doesn't notice when you declare a variable but don't use it.

Correct, Clang is missing this warning. It would make a great addition!

I wrote this simple testcase and put it in

llvm/tools/clang/test/Sema/warning-unused-variables.c

---
// RUN: clang -fsyntax-only -Wunused-variables -verify %s

void f0(void) {
  int a, b; // expected-warning{{unused-variable}}
  a = 10;
  return;
}
---

And sure enough:

$ clang-cc -Wunused-variables -fsyntax-only -verify warn-unused-
variables.c
Warnings expected but not seen:
  Line 4: unused-variable
Warnings seen but not expected:
  Line 0: unknown warning option '-Wunused-variables'

Obviously there is no -Wunused-variables flag or unused variables
warning yet because that's what I need to implement. So my question
is, what's the best way start hacking on this? Is there a document
that describes the AST and where the warnings are generated? What
files should I start reading?

Generally, the best way to figure out how to add a new warning like this is to find a similar warning. Clang has -Wunused-parameter already implemented, so that's a great start. There are a few places you'll need to look at and (in some cases) modify to implement -Wunused-variables:

  include/clang/Basic/DiagnosticSemaKinds.td: define a new warn_unused_variable warning here
  lib/Sema/Sema.h: DiagnoseUnusedParameters shows how to emit a similar warning
  lib/Sema/SemaDecl.cpp: Sema::ActOnPopScope is where you probably want to look for unused variables (when they are being popped out of scope)
      lib/Sema/SemaExpr.cpp: Sema::MarkDeclarationReferenced is called whenever any declaration (variable, function, method, etc.) is used by the source code.

Most of the documentation for the ASTs is within the AST headers, which is also accessible via Doxygen:

  http://clang.llvm.org/doxygen/

  - Doug

Okay, this seems to do it:

ob.patch (1.08 KB)

warn-unused-variables.c (152 Bytes)

FWIW, I found an open bug

http://llvm.org/bugs/show_bug.cgi?id=4563

so I added the patch there as well.

Cheers,

-Oscar

Hm, after playing with my patch a little I see it's not quite right.

struct s1 {
  unsigned int i;
};

will produce an "unused variable 'i'" warning. This is a better patch.

(I'm also adding it to the bug-tracker)

ob.patch (1.12 KB)

warn-unused-variables.c (211 Bytes)

Hm, after playing with my patch a little I see it's not quite right.

struct s1 {
  unsigned int i;
};

will produce an "unused variable 'i'" warning. This is a better patch.

This condition:

+ if (isa<VarDecl>(D) && !isa<ParmVarDecl>(D) &&
+ !D->isUsed() && !D->hasAttr<UnusedAttr>())
+ Diag(D->getLocation(), diag::warn_unused_variable) << D- >getDeclName();

is a good start, but I think you only want to complain about function-local variables, e.g., by checking that

  D->getDeclContext()->isFunctionOrMethod()

There are a few other issues that need to be resolved:

   1) If a variable is only used in an unevaluated context (such as the operand of a sizeof expression), it will incorrectly complain that the variable is unused:

void g() {
   int i;
   (void)sizeof(i);
}

   2) It incorrectly complains about variables in templates:

template<typename T> void f() {
   T t;
   t = 17;
}

With this warning, it makes sense to enable the warning when compiling a small-to-medium---sized C application and then look at where the warning triggers. Did it complain too often? Did GCC correctly complain about something that Clang missed?

  - Doug

That's how I found my previous error, I compiled a product that I'm working on and got all sorts of warnings. The problem is that "normal" code doesn't have unused variables, so you have to artificially introduce them or compile new code with it.

My first thought was that parameters should have the same problem, but they are special-cased in MarkDeclaraionReferenced(). I added a special case for Variables too, is this right?

Here's what I have:

The patch:

ob.patch (1.91 KB)

warn-unused-variables.c (265 Bytes)

warn-unused-variables.cpp (107 Bytes)

Still not quite right:

$ clang-cc test/SemaObjC/unused.m -verify -Wunused -fsyntax-only
Warnings seen but not expected:
   Line 0: unused variable 'self'
   Line 0: unused variable '_cmd'
   Line 0: unused variable 'self'
   Line 0: unused variable '_cmd'
   Line 41: unused variable 'y'

Sigh. I'll look at this tomorrow...

Cheers,

-Oscar

Ok, this one seems to do the trick:

ob.patch (2.03 KB)

warn-unused-variables.c (268 Bytes)

warn-unused-variables.cpp (107 Bytes)

Is this any better?

Still not quite right:

Ok, this one seems to do the trick:

<ob.patch>

Testcases:

<warn-unused-variables.c><warn-unused-variables.cpp>

I ran "make test" and only got one failure:

FAIL: Clang::Index/c-index-api-test.m (646 of 1636)

which seems unrelated. I also compiled Adium with it and didn't get any obvious false positives or false negatives.

Would it be better to write the if in SemaDecl.cpp

     if (isa<VarDecl>(D) && !isa<ParmVarDecl>(D) && !isa<ImplicitParamDecl>(D) &&
  D->getDeclContext()->isFunctionOrMethod() &&
  !D->isUsed() && !D->hasAttr<UnusedAttr>()) {
      Diag(D->getLocation(), diag::warn_unused_variable) << D- >getDeclName();
     }

as

     if (!D->isUsed() && !D->hasAttr<UnusedAttr>() &&
  isa<VarDecl>(D) && !isa<ParmVarDecl>(D) && !isa<ImplicitParamDecl>(D) &&
  D->getDeclContext()->isFunctionOrMethod()) {
      Diag(D->getLocation(), diag::warn_unused_variable) << D- >getDeclName();
     }

i.e. move the "not used and no unused attr" to the beginning to take advantage of short-circuit logic?

or is this kind of micro-optimizing frowned upon? :slight_smile:

Cheers,

-Oscar

Wonderful, thank you! I've committed this patch here:

  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20091005/022108.html

along with your suggested micro-optimization. We love optimizations of all kinds, and moving the isUsed() check first will save a lot of isa<> checks. Good catch!

  - Doug

Thanks. I have some other warning fixes that I'll post here (is here the best place?).

Also, should I close the bug? http://llvm.org/bugs/show_bug.cgi?id=4563 or is there someone responsible for that?

Cheers,

-Oscar

Thanks. I have some other warning fixes that I'll post here (is here the best place?).

Generally, patches should go to cfe-commits, and someone will review them there.

Also, should I close the bug? http://llvm.org/bugs/show_bug.cgi?id=4563 or is there someone responsible for that?

Yes, you can go ahead and close the bug. Thanks!

  - Doug