[patch] Implementing -Wunused-local-typedefs

[Richard Smith]

 def UnusedConstVariable : DiagGroup<"unused-const-variable">;
 def UnusedVariable : DiagGroup<"unused-variable",
                                [UnusedConstVariable]>;
+def UnusedLocalTypedefs : DiagGroup<"unused-local-typedefs">;
 def UnusedPropertyIvar :  DiagGroup<"unused-property-ivar">;

I'm a little uncomfortable about this flag being plural and the others
being singular. GCC compatible, you say? =(


+def warn_unused_local_typedefs : Warning<"unused typedef %0">,

This one should definitely be singular.

Reformatting of lib/Parse/ParseExprCXX.cpp looks unrelated. Feel free to
commit that separately.

+    if (TypedefDecl* TD = dyn_cast_or_null<TypedefDecl>(SD))

* should go on the right. Also, use 'auto' for a variable initialized from
cast/dyn_cast/etc.

Rather than calling setReferenced directly, please call
Sema::MarkAnyDeclReferenced.

+static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D,
+                                     const DeclContext *DC) {
[...]
+  if (!DC->isFunctionOrMethod())
+    return false;

Please document what 'DC' is here; it's really not obvious. But it seems to
me that you can replace DC here and below with just a bool WithinFunction.
You can also remove it entirely and check whether D->getDeclContext() is a
function, method, or local class.

+void Sema::DiagnoseUnusedDecl(const NamedDecl *D, const DeclContext *DC) {
+  if (DC->isFunctionOrMethod())
+    if (const RecordDecl *RD = dyn_cast<RecordDecl>(D))
+      for (auto *TmpD : RD->decls())
+        if (isa<TypedefDecl>(TmpD) || isa<RecordDecl>(TmpD))
+          DiagnoseUnusedDecl(cast<NamedDecl>(TmpD), DC);

This would make more sense as a layer between ActOnPopScope and
DiagnoseUnusedDecl. A comment explaining why we only do this within a
function or method would help ("within a function or method, we defer these
checks until the end of the surrounding scope").

Also, this isn't necessarily correct in C++14, since a local type's members
can be first used once we've already left the function:

  auto f() {
    struct S { typedef int t; };
    return S();
  }
  auto x = f();
  decltype(x)::t y;

What happens if the local type is used as an argument to a function
template, which in turn uses the typedef? Can we really diagnose this
before end-of-TU?

+  else if (isa<TypedefDecl>(D))  // TODO: Warn on unused c++11 aliases too?
+    DiagID = diag::warn_unused_local_typedefs;

Yes, please change this to TypedefNameDecl (throughout the patch). It
doesn't make sense to handle these differently, since one can be a
redeclaration of the other.

Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp (revision 207920)
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp (working copy)
@@ -3651,7 +3651,7 @@
   if (!NewVar->isInvalidDecl() &&
       NewVar->getDeclContext()->isFunctionOrMethod() && !NewVar->isUsed()
&&
       OldVar->getType()->isDependentType())
-    DiagnoseUnusedDecl(NewVar);
+    DiagnoseUnusedDecl(NewVar, NewVar->getDeclContext());

Hmm, why do we diagnose unused decls from template instantiation at all?

On Sat, May 3, 2014 at 10:37 PM, Nico Weber <thakis at chromium.org> wrote:

> With one more setReferenced() call (in SemaCXXScopeSpec.cpp). This
> version has successfully compiled several thousand files of chromium
> (all that I've tried so far).
>
> On Sat, May 3, 2014 at 6:35 PM, Nico Weber <thakis at chromium.org> wrote:
> > About point 2: The patch currently incorrectly warns on the "v" typedef
> in:
> >
> >   template <class T> struct V { typedef int iterator; };
> >   void f() {
> >     typedef V<int> v;
> >     v::iterator it;
> >   }
> >
> > So there's at least one more place where I need to insert a
> > setReferenced() I'll send an updated version once I found where, but
> > I'm thankful for comments on the overall approach in the meantime too.
> >
> > On Sat, May 3, 2014 at 6:08 PM, Nico Weber <thakis at chromium.org> wrote:
> >> (Forgot to say: This is also PR18265.)
> >>
> >> On Sat, May 3, 2014 at 6:07 PM, Nico Weber <thakis at chromium.org> wrote:
> >>> Hi,
> >>>
> >>> gcc has a warning -Wunused-local-typedefs that warns on unused local
> >>> typedefs. Inspired by r207870, I thought it'd be nice if clang had
> >>> this warning too. This warning requires the following three things:
> >>>
> >>>
> >>> 1.) A bit to track if a typedef has been referenced.
> >>>
> >>> Decl already has isUsed and isReferenced, but they don't seem to be
> >>> used for TypedefDecls, so I'm just using isReferenced on TypedefDecls
> >>> for this.
> >>>
> >>>
> >>> 2.) Setting that bit on typedefs that are used.
> >>>
> >>> The three strategies I can think of:
> >>> a.) Do this in Sema::DiagnoseUseOfDecl(), this seems to be already
> >>> called for decls all over the place, and an "if isa<TypedefDecl>(D)
> >>> D->setReferenced()" seems to do the trick.
> >>> b.) Do this in LookupName
> >>> c.) Do this explicitly in the places where it's actually necessary.
> >>>
> >>> The attached patch goes with the last approach. The three places I
> >>> found where it's needed are Sema::getTypeName(), Sema::ClassifyName(),
> >>> and Sema::LookupInlineAsmField(). The first two are called by the
> >>> parser for almost everything, while the third is called for references
> >>> to structs from MS inline assembly. That last one is a bit scary as it
> >>> means it's possible that there are other places this is needed that I
> >>> missed.
> >>>
> >>>
> >>> 3.) A way to check all typedefs in a scope when that scope ends.
> >>>
> >>> I added this to Sema::ActOnPopScope() which is where the
> >>> unused-variable and unused-label warnings are created. This works
> >>> well, but to catch the unused local typedef in
> >>>
> >>>   {
> >>>     struct A {
> >>>       struct B { typedef int SoDeep; };
> >>>     };
> >>>   }
> >>>
> >>> it means that that code now needs to recurse into all RecordDecl in a
> >>> scope, which it didn't need to do previously.
> >>>
> >>>
> >>> Let me know how badly I've chosen in each instance :-)
> >>>
> >>> Thanks,
> >>> Nico
> >>>
> >>> ps: If this goes in, a follow-up question is if this should warn on
> >>> unused C++11 type aliases too – it'd just require
> >>> s/TypedefDecl/TypedefNameDecl/ in two places in the patch, so it
> >>> should be an easy follow-up.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140506/16340b90/attachment.html>