[patch] Implementing -Wunused-local-typedefs

[Richard Smith]

On Tue, Jun 24, 2014 at 9:55 AM, Nico Weber <thakis at chromium.org> wrote:

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

Generally, yes, it would. The difference here is that we never want to
diagnose unused typedefs that are defined in a module, so we can choose to
just not serialize the list in that case.


>
>
>>
>> +  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/20140627/acbf9950/attachment.html>