[patch] Implementing -Wunused-local-typedefs
Alp Toker
alp at nuanti.com
Sun May 4 00:15:30 PDT 2014
On 04/05/2014 06:37, Nico Weber 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.
>
> clang-unused-local-typedefs.diff
>
>
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td (revision 207920)
> +++ include/clang/Basic/DiagnosticGroups.td (working copy)
> @@ -382,6 +382,7 @@
> def UnusedConstVariable : DiagGroup<"unused-const-variable">;
> def UnusedVariable : DiagGroup<"unused-variable",
> [UnusedConstVariable]>;
> +def UnusedLocalTypedefs : DiagGroup<"unused-local-typedefs">;
> def UnusedPropertyIvar : DiagGroup<"unused-property-ivar">;
> def UsedButMarkedUnused : DiagGroup<"used-but-marked-unused">;
> def UserDefinedLiterals : DiagGroup<"user-defined-literals">;
> @@ -502,7 +503,7 @@
> [UnusedArgument, UnusedFunction, UnusedLabel,
> // UnusedParameter, (matches GCC's behavior)
> // UnusedMemberFunction, (clean-up llvm before enabling)
> - UnusedPrivateField,
> + UnusedPrivateField, UnusedLocalTypedefs,
> UnusedValue, UnusedVariable, UnusedPropertyIvar]>,
> DiagCategory<"Unused Entity Issue">;
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 207920)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -167,6 +167,8 @@
> InGroup<UnusedParameter>, DefaultIgnore;
> def warn_unused_variable : Warning<"unused variable %0">,
> InGroup<UnusedVariable>, DefaultIgnore;
> +def warn_unused_local_typedefs : Warning<"unused typedef %0">,
> + InGroup<UnusedLocalTypedefs>, DefaultIgnore;
> def warn_unused_property_backing_ivar :
> Warning<"ivar %0 which backs the property is not "
> "referenced in this property's accessor">,
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h (revision 207920)
> +++ include/clang/Sema/Sema.h (working copy)
> @@ -3158,7 +3158,7 @@
> /// DiagnoseUnusedExprResult - If the statement passed in is an expression
> /// whose result is unused, warn.
> void DiagnoseUnusedExprResult(const Stmt *S);
> - void DiagnoseUnusedDecl(const NamedDecl *ND);
> + void DiagnoseUnusedDecl(const NamedDecl *ND, const DeclContext *DC);
>
> /// Emit \p DiagID if statement located on \p StmtLoc has a suspicious null
> /// statement as a \p Body, and it is located on the same line.
> Index: lib/Parse/ParseExprCXX.cpp
> ===================================================================
> --- lib/Parse/ParseExprCXX.cpp (revision 207920)
> +++ lib/Parse/ParseExprCXX.cpp (working copy)
> @@ -426,8 +426,8 @@
>
> if (Next.is(tok::coloncolon)) {
> if (CheckForDestructor && GetLookAheadToken(2).is(tok::tilde) &&
> - !Actions.isNonTypeNestedNameSpecifier(getCurScope(), SS, Tok.getLocation(),
> - II, ObjectType)) {
> + !Actions.isNonTypeNestedNameSpecifier(
> + getCurScope(), SS, Tok.getLocation(), II, ObjectType)) {
> *MayBePseudoDestructor = true;
> return false;
> }
> Index: lib/Sema/SemaCXXScopeSpec.cpp
> ===================================================================
> --- lib/Sema/SemaCXXScopeSpec.cpp (revision 207920)
> +++ lib/Sema/SemaCXXScopeSpec.cpp (working copy)
> @@ -614,6 +614,9 @@
> }
> }
>
> + if (TypedefDecl* TD = dyn_cast_or_null<TypedefDecl>(SD))
> + TD->setReferenced();
> +
> // If we're just performing this lookup for error-recovery purposes,
> // don't extend the nested-name-specifier. Just return now.
> if (ErrorRecoveryLookup)
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp (revision 207922)
> +++ lib/Sema/SemaDecl.cpp (working copy)
> @@ -310,6 +310,7 @@
> DiagnoseUseOfDecl(IIDecl, NameLoc);
>
> T = Context.getTypeDeclType(TD);
> + TD->setReferenced();
>
> // NOTE: avoid constructing an ElaboratedType(Loc) if this is a
> // constructor or destructor name (in such a case, the scope specifier
> @@ -813,6 +814,7 @@
> NamedDecl *FirstDecl = (*Result.begin())->getUnderlyingDecl();
> if (TypeDecl *Type = dyn_cast<TypeDecl>(FirstDecl)) {
> DiagnoseUseOfDecl(Type, NameLoc);
> + Type->setReferenced();
> QualType T = Context.getTypeDeclType(Type);
> if (SS.isNotEmpty())
> return buildNestedType(*this, SS, T, NameLoc);
> @@ -1268,7 +1270,8 @@
> UnusedFileScopedDecls.push_back(D);
> }
>
> -static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D) {
> +static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D,
> + const DeclContext *DC) {
> if (D->isInvalidDecl())
> return false;
>
> @@ -1278,10 +1281,15 @@
>
> if (isa<LabelDecl>(D))
> return true;
> +
> + if (!DC->isFunctionOrMethod())
> + return false;
> +
> + if (isa<TypedefDecl>(D))
> + return true;
>
> // White-list anything that isn't a local variable.
> - if (!isa<VarDecl>(D) || isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D) ||
> - !D->getDeclContext()->isFunctionOrMethod())
> + if (!isa<VarDecl>(D) || isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D))
> return false;
>
> // Types of valid local variables should be complete, so this should succeed.
> @@ -1346,8 +1354,14 @@
>
> /// DiagnoseUnusedDecl - Emit warnings about declarations that are not used
> /// unless they are marked attr(unused).
> -void Sema::DiagnoseUnusedDecl(const NamedDecl *D) {
> - if (!ShouldDiagnoseUnusedDecl(D))
> +void Sema::DiagnoseUnusedDecl(const NamedDecl *D, const DeclContext *DC) {
Nice. Small suggestion: I'd go for a default argument DC = 0, and if
(!DC) DC = D->getDeclContext() to keep the call signature simple here.
Alp.
> + 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);
> +
> + if (!ShouldDiagnoseUnusedDecl(D, DC))
> return;
>
> FixItHint Hint;
> @@ -1358,6 +1372,8 @@
> DiagID = diag::warn_unused_exception_param;
> else if (isa<LabelDecl>(D))
> DiagID = diag::warn_unused_label;
> + else if (isa<TypedefDecl>(D)) // TODO: Warn on unused c++11 aliases too?
> + DiagID = diag::warn_unused_local_typedefs;
> else
> DiagID = diag::warn_unused_variable;
>
> @@ -1389,7 +1405,7 @@
>
> // Diagnose unused variables in this scope.
> if (!S->hasUnrecoverableErrorOccurred())
> - DiagnoseUnusedDecl(D);
> + DiagnoseUnusedDecl(D, D->getDeclContext());
>
> // If this was a forward reference to a label, verify it was defined.
> if (LabelDecl *LD = dyn_cast<LabelDecl>(D))
> Index: lib/Sema/SemaStmtAsm.cpp
> ===================================================================
> --- lib/Sema/SemaStmtAsm.cpp (revision 207920)
> +++ lib/Sema/SemaStmtAsm.cpp (working copy)
> @@ -441,8 +441,10 @@
> NamedDecl *FoundDecl = BaseResult.getFoundDecl();
> if (VarDecl *VD = dyn_cast<VarDecl>(FoundDecl))
> RT = VD->getType()->getAs<RecordType>();
> - else if (TypedefDecl *TD = dyn_cast<TypedefDecl>(FoundDecl))
> + else if (TypedefDecl *TD = dyn_cast<TypedefDecl>(FoundDecl)) {
> + TD->setReferenced();
> RT = TD->getUnderlyingType()->getAs<RecordType>();
> + }
> if (!RT)
> return true;
>
> 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());
> }
>
> /// \brief Instantiate the initializer of a variable.
> Index: test/SemaCXX/implicit-exception-spec.cpp
> ===================================================================
> --- test/SemaCXX/implicit-exception-spec.cpp (revision 207920)
> +++ test/SemaCXX/implicit-exception-spec.cpp (working copy)
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -std=c++11 -Wall %s
> +// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -verify -std=c++11 -Wall -Wno-unused-local-typedefs %s
>
> template<bool b> struct ExceptionIf { static int f(); };
> template<> struct ExceptionIf<false> { typedef int f; };
> Index: test/SemaCXX/warn-unused-filescoped.cpp
> ===================================================================
> --- test/SemaCXX/warn-unused-filescoped.cpp (revision 207920)
> +++ test/SemaCXX/warn-unused-filescoped.cpp (working copy)
> @@ -1,5 +1,5 @@
> -// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-member-function -Wno-c++11-extensions -std=c++98 %s
> -// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-member-function -std=c++11 %s
> +// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-member-function -Wno-unused-local-typedefs -Wno-c++11-extensions -std=c++98 %s
> +// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-member-function -Wno-unused-local-typedefs -std=c++11 %s
>
> #ifdef HEADER
>
> Index: test/SemaCXX/warn-unused-local-typedefs.cpp
> ===================================================================
> --- test/SemaCXX/warn-unused-local-typedefs.cpp (revision 0)
> +++ test/SemaCXX/warn-unused-local-typedefs.cpp (working copy)
> @@ -0,0 +1,103 @@
> +// RUN: %clang_cc1 -Wunused-local-typedefs -fasm-blocks -verify -std=c++11 %s
> +
> +struct S {
> + typedef int Foo; // no diag
> +};
> +
> +namespace N {
> + typedef int Foo; // no diag
> + typedef int Foo2; // no diag
> +}
> +
> +template <class T> class Vec {};
> +
> +typedef int global_foo; // no diag (?)
> +
> +void f() {
> + typedef int foo0; // expected-warning {{unused typedef 'foo0'}}
> +
> + typedef int foo1 __attribute__((unused)); // no diag
> +
> + typedef int foo2;
> + {
> + typedef int foo2; // expected-warning {{unused typedef 'foo2'}}
> + }
> + typedef foo2 foo3; // expected-warning {{unused typedef 'foo3'}}
> +
> + typedef int foo2_2; // expected-warning {{unused typedef 'foo2_2'}}
> + {
> + typedef int foo2_2;
> + typedef foo2_2 foo3_2; // expected-warning {{unused typedef 'foo3_2'}}
> + }
> +
> + typedef int foo4;
> + foo4 the_thing;
> +
> + typedef int* foo5;
> + typedef foo5* foo6; // no diag
> + foo6 *myptr;
> +
> + struct S2 {
> + typedef int Foo, Foo2; // expected-warning {{unused typedef 'Foo2'}}
> +
> + struct Deeper {
> + typedef int DeepFoo; // expected-warning {{unused typedef 'DeepFoo'}}
> + };
> + };
> +
> + S2::Foo s2foo;
> +
> + typedef struct {} foostruct; // expected-warning {{unused typedef 'foostruct'}}
> +
> + typedef struct {} foostruct2; // no diag
> + foostruct2 fs2;
> +
> + typedef int vecint; // no diag
> + Vec<vecint> v;
> +
> + N::Foo nfoo;
> +
> + typedef int cint; // no diag
> + constexpr int kFoo = (int)(cint)5;
> +
> + typedef struct {
> + int a;
> + int b;
> + } A; // no diag
> + __asm mov eax, [eax].A.b
> +}
> +
> +int printf(char const *, ...);
> +
> +void test() {
> + typedef signed long int superint; // no diag
> + printf("%f", (superint) 42);
> +
> + typedef signed long int superint2; // no diag
> + printf("%f", static_cast<superint2>(42));
> +}
> +
> +template<typename T>
> +void g() {
> + typedef T foo; // expected-warning{{unused typedef 'foo'}}
> +}
> +
> +void t5() {
> + class A {
> + };
> + typedef A tA;
> + typedef A tA2; // expected-warning{{unused typedef 'tA2'}}
> + class B {
> + friend tA;
> + };
> +}
> +
> +template<class T>
> +struct V {
> + typedef int iterator;
> +};
> +
> +void chain() {
> + typedef V<int> v; // no diag
> + v::iterator it;
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list