On Tue, Jun 24, 2014 at 9:55 AM, Nico Weber <[email protected]> wrote:
> 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. 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 <[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
