[patch] Implementing -Wunused-local-typedefs
Nico Weber
thakis at chromium.org
Wed Aug 6 18:08:57 PDT 2014
ping
On Sun, Jul 27, 2014 at 8:36 PM, Nico Weber <thakis at chromium.org> wrote:
> This now also warns on
>
> template <class T>
> void template_fun(T t) {
> struct S2 {
> typedef int Foo1;
> };
> }
> void template_fun_user() {
> template_fun(3);
> }
>
> That is, it fixes the FIXME mentioned in the last post (by checking for
> unused typedefs in TemplateDeclInstantiator::VisitCXXRecordDecl after the
> record decl has been instantiated). I also
> changed ShouldDiagnoseUnusedDecl() to not take a new parameter but instead
> compute WithinFunction locally like you suggested.
>
> (I also removed a redundant IsUsed() check
> in Sema::BuildVariableInstantiation() – DiagnoseUnusedDecl() checks that
> already.)
>
> I'll stop futzing with this now; I think this is read for a look. (Not
> urgent at all, of course.)
>
>
> On Sun, Jul 27, 2014 at 5:13 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> Now gets this right:
>>
>> template <class T>
>> void template_fun(T t) {
>> typename T::Foo s3foo;
>> }
>> void template_fun_user() {
>> struct Local {
>> typedef int Foo; // no-diag
>> typedef int Bar; // expected-warning {{unused typedef 'Bar'}}
>> } p;
>> template_fun(p);
>> }
>>
>> It also no longer warns on S::Foo in:
>>
>> template<class T>
>> void tf(T t) {
>> struct S {
>> typedef int Foo;
>> typedef int Bar;
>> };
>> S::Foo f;
>> }
>>
>> (It stopped warning on S::Bar too – there's a FIXME in the test that
>> explains why and how to fix that.)
>>
>>
>>
>> On Sat, Jul 26, 2014 at 9:26 PM, Nico Weber <thakis at chromium.org> wrote:
>>
>>> I'm delighted to announce that, after almost two months of hard work, a
>>> new patch set is available for review!
>>>
>>> With these tantalizing features and fixes:
>>> * TypedefNameDecls everywhere
>>> * MarkAnyDeclReferenced() used throughout
>>> * Instead of immediately emitting a diag, the TypedefNameDecls are
>>> stashed in a set and the diagnostics for things in the set are emitted at
>>> end-of-TU (except for TypedefNameDecls that have been referenced since then)
>>> * It's now called -Wunused-local-typedef in singular, with an alias for
>>> the gcc spelling
>>> * More tests
>>>
>>> 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.
>>>>
>>>
>>> Done.
>>>
>>>
>>>> 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.
>>>>
>>>
>>> Done.
>>>
>>>
>>>> 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?
>>>>
>>>
>>> Done.
>>>
>>>
>>>> + 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.
>>>>
>>>
>>> Done.
>>>
>>>
>>>>
>>>> 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?
>>>>
>>>
>>> r103362 added this, r133580 tweaked it some. Can you think of an example
>>> where a template-local typedef is unused only in some instantiations of
>>> that template?
>>>
>>>
>>>>
>>>> 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/20140806/d4108b68/attachment.html>
More information about the cfe-commits
mailing list