[patch] Implementing -Wunused-local-typedefs

[Nico Weber]

On Tue, May 6, 2014 at 6:01 PM, Richard Smith <richard at metafoo.co.uk> wrote:

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

Sneaky! gcc gets this wrong too (filed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61596). This kills the
"local-" part of the warning.

Is diagnosing this at end of TU fine? I vaguely recall that a concern with
-Wunused-private-field was that it forced deserialization of many ast nodes
(?); would that be a concern here too?


>
> +  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/20140624/84c2fabf/attachment.html>