ping
On Sun, Jul 27, 2014 at 8:36 PM, Nico Weber <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>> 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 <[email protected]> >>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>> wrote: >>>>> >> (Forgot to say: This is also PR18265.) >>>>> >> >>>>> >> On Sat, May 3, 2014 at 6:07 PM, Nico Weber <[email protected]> >>>>> 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. >>>>> >>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
