From f8cc02e50553b5c3bc6570bff0c47ac7db85fe8d Mon Sep 17 00:00:00 2001 From: Daniel Jasper Date: Wed, 6 Jun 2012 08:32:04 +0000 Subject: [PATCH] Introduce -Wunused-private-field. If enabled, this warning detects unused private fields of classes that are fully defined in the current translation unit. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@158054 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 2 + include/clang/Basic/DiagnosticSemaKinds.td | 2 + include/clang/Sema/Sema.h | 6 + lib/Sema/Sema.cpp | 91 ++++++++++ lib/Sema/SemaDeclCXX.cpp | 56 +++++- lib/Sema/SemaExprMember.cpp | 2 + test/SemaCXX/warn-unused-member.cpp | 193 +++++++++++++++++++++ 7 files changed, 350 insertions(+), 2 deletions(-) create mode 100644 test/SemaCXX/warn-unused-member.cpp diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index f48eee416a..fdafb99109 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -230,6 +230,7 @@ def UnusedComparison : DiagGroup<"unused-comparison">; def UnusedExceptionParameter : DiagGroup<"unused-exception-parameter">; def UnneededInternalDecl : DiagGroup<"unneeded-internal-declaration">; def UnneededMemberFunction : DiagGroup<"unneeded-member-function">; +def UnusedPrivateField : DiagGroup<"unused-private-field">; def UnusedFunction : DiagGroup<"unused-function", [UnneededInternalDecl]>; def UnusedMemberFunction : DiagGroup<"unused-member-function", [UnneededMemberFunction]>; @@ -311,6 +312,7 @@ def Unused : DiagGroup<"unused", [UnusedArgument, UnusedFunction, UnusedLabel, // UnusedParameter, (matches GCC's behavior) // UnusedMemberFunction, (clean-up llvm before enabling) + // UnusedPrivateField, (clean-up llvm before enabling) UnusedValue, UnusedVariable]>, DiagCategory<"Unused Entity Issue">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 7ed00e6b52..3dc03fa802 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -173,6 +173,8 @@ def warn_unneeded_internal_decl : Warning< def warn_unneeded_member_function : Warning< "member function %0 is not needed and will not be emitted">, InGroup, DefaultIgnore; +def warn_unused_private_field: Warning<"private field %0 is not used">, + InGroup, DefaultIgnore; def warn_parameter_size: Warning< "%0 is a large (%1 bytes) pass-by-value argument; " diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index d7bc2bfe19..f0d3213bd3 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -37,6 +37,7 @@ #include "clang/Basic/ExpressionTraits.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/OwningPtr.h" +#include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include @@ -267,6 +268,11 @@ public: /// FieldCollector - Collects CXXFieldDecls during parsing of C++ classes. OwningPtr FieldCollector; + typedef llvm::SmallSetVector NamedDeclSetType; + + /// \brief Set containing all declared private fields that are not used. + NamedDeclSetType UnusedPrivateFields; + typedef llvm::SmallPtrSet RecordDeclSetTy; /// PureVirtualClassDiagSet - a set of class declarations which we have diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp index 9f2880fb4e..e0d405d175 100644 --- a/lib/Sema/Sema.cpp +++ b/lib/Sema/Sema.cpp @@ -30,6 +30,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" #include "clang/AST/DeclCXX.h" +#include "clang/AST/DeclFriend.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" @@ -422,6 +423,79 @@ void Sema::LoadExternalWeakUndeclaredIdentifiers() { } } + +typedef llvm::DenseMap RecordCompleteMap; + +/// \brief Returns true, if all methods and nested classes of the given +/// CXXRecordDecl are defined in this translation unit. +/// +/// Should only be called from ActOnEndOfTranslationUnit so that all +/// definitions are actually read. +static bool MethodsAndNestedClassesComplete(const CXXRecordDecl *RD, + RecordCompleteMap &MNCComplete) { + RecordCompleteMap::iterator Cache = MNCComplete.find(RD); + if (Cache != MNCComplete.end()) + return Cache->second; + if (!RD->isCompleteDefinition()) + return false; + bool Complete = true; + for (DeclContext::decl_iterator I = RD->decls_begin(), + E = RD->decls_end(); + I != E && Complete; ++I) { + if (const CXXMethodDecl *M = dyn_cast(*I)) + Complete = M->isDefined() || (M->isPure() && !isa(M)); + else if (const CXXRecordDecl *R = dyn_cast(*I)) { + if (R->isInjectedClassName()) + continue; + if (R->hasDefinition()) + Complete = MethodsAndNestedClassesComplete(R->getDefinition(), + MNCComplete); + else + Complete = false; + } + } + MNCComplete[RD] = Complete; + return Complete; +} + +/// \brief Returns true, if the given CXXRecordDecl is fully defined in this +/// translation unit, i.e. all methods are defined or pure virtual and all +/// friends, friend functions and nested classes are fully defined in this +/// translation unit. +/// +/// Should only be called from ActOnEndOfTranslationUnit so that all +/// definitions are actually read. +static bool IsRecordFullyDefined(const CXXRecordDecl *RD, + RecordCompleteMap &RecordsComplete, + RecordCompleteMap &MNCComplete) { + RecordCompleteMap::iterator Cache = RecordsComplete.find(RD); + if (Cache != RecordsComplete.end()) + return Cache->second; + bool Complete = MethodsAndNestedClassesComplete(RD, MNCComplete); + for (CXXRecordDecl::friend_iterator I = RD->friend_begin(), + E = RD->friend_end(); + I != E && Complete; ++I) { + // Check if friend classes and methods are complete. + if (TypeSourceInfo *TSI = (*I)->getFriendType()) { + // Friend classes are available as the TypeSourceInfo of the FriendDecl. + if (CXXRecordDecl *FriendD = TSI->getType()->getAsCXXRecordDecl()) + Complete = MethodsAndNestedClassesComplete(FriendD, MNCComplete); + else + Complete = false; + } else { + // Friend functions are available through the NamedDecl of FriendDecl. + if (const FunctionDecl *FD = + dyn_cast((*I)->getFriendDecl())) + Complete = FD->isDefined(); + else + // This is a template friend, give up. + Complete = false; + } + } + RecordsComplete[RD] = Complete; + return Complete; +} + /// ActOnEndOfTranslationUnit - This is called at the very end of the /// translation unit when EOF is reached and all but the top-level scope is /// popped. @@ -628,6 +702,23 @@ void Sema::ActOnEndOfTranslationUnit() { checkUndefinedInternals(*this); } + if (Diags.getDiagnosticLevel(diag::warn_unused_private_field, + SourceLocation()) + != DiagnosticsEngine::Ignored) { + RecordCompleteMap RecordsComplete; + RecordCompleteMap MNCComplete; + for (NamedDeclSetType::iterator I = UnusedPrivateFields.begin(), + E = UnusedPrivateFields.end(); I != E; ++I) { + const NamedDecl *D = *I; + const CXXRecordDecl *RD = dyn_cast(D->getDeclContext()); + if (RD && !RD->isUnion() && + IsRecordFullyDefined(RD, RecordsComplete, MNCComplete)) { + Diag(D->getLocation(), diag::warn_unused_private_field) + << D->getDeclName(); + } + } + } + // Check we've noticed that we're no longer parsing the initializer for every // variable. If we miss cases, then at best we have a performance issue and // at worst a rejects-valid bug. diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index 676063ac08..e7a22165da 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -1439,6 +1439,17 @@ bool Sema::CheckIfOverriddenFunctionIsMarkedFinal(const CXXMethodDecl *New, return true; } +static bool InitializationHasSideEffects(const FieldDecl &FD) { + if (!FD.getType().isNull()) { + if (const CXXRecordDecl *RD = FD.getType()->getAsCXXRecordDecl()) { + return !RD->isCompleteDefinition() || + !RD->hasTrivialDefaultConstructor() || + !RD->hasTrivialDestructor(); + } + } + return false; +} + /// ActOnCXXMemberDeclarator - This is invoked when a C++ class member /// declarator is parsed. 'AS' is the access specifier, 'BW' specifies the /// bitfield width if there is one, 'InitExpr' specifies the initializer if @@ -1624,8 +1635,23 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D, assert((Name || isInstField) && "No identifier for non-field ?"); - if (isInstField) - FieldCollector->Add(cast(Member)); + if (isInstField) { + FieldDecl *FD = cast(Member); + FieldCollector->Add(FD); + + if (Diags.getDiagnosticLevel(diag::warn_unused_private_field, + FD->getLocation()) + != DiagnosticsEngine::Ignored) { + // Remember all explicit private FieldDecls that have a name, no side + // effects and are not part of a dependent type declaration. + if (!FD->isImplicit() && FD->getDeclName() && + FD->getAccess() == AS_private && + !FD->getParent()->getTypeForDecl()->isDependentType() && + !InitializationHasSideEffects(*FD)) + UnusedPrivateFields.insert(FD); + } + } + return Member; } @@ -2105,6 +2131,25 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr *Init, Args = InitList->getInits(); NumArgs = InitList->getNumInits(); } + + // Mark FieldDecl as being used if it is a non-primitive type and the + // initializer does not call the default constructor (which is trivial + // for all entries in UnusedPrivateFields). + // FIXME: Make this smarter once more side effect-free types can be + // determined. + if (NumArgs > 0) { + if (Member->getType()->isRecordType()) { + UnusedPrivateFields.remove(Member); + } else { + for (unsigned i = 0; i < NumArgs; ++i) { + if (Args[i]->HasSideEffects(Context)) { + UnusedPrivateFields.remove(Member); + break; + } + } + } + } + for (unsigned i = 0; i < NumArgs; ++i) { SourceLocation L; if (InitExprContainsUninitializedFields(Args[i], Member, &L)) { @@ -2808,6 +2853,13 @@ static bool CollectFieldInitializer(Sema &SemaRef, BaseAndFieldInfo &Info, SourceLocation(), 0, SourceLocation()); Info.AllToInit.push_back(Init); + + // Check whether this initializer makes the field "used". + Expr *InitExpr = Field->getInClassInitializer(); + if (Field->getType()->isRecordType() || + (InitExpr && InitExpr->HasSideEffects(SemaRef.Context))) + SemaRef.UnusedPrivateFields.remove(Field); + return false; } diff --git a/lib/Sema/SemaExprMember.cpp b/lib/Sema/SemaExprMember.cpp index f8bf908dfc..d888ab90b6 100644 --- a/lib/Sema/SemaExprMember.cpp +++ b/lib/Sema/SemaExprMember.cpp @@ -1591,6 +1591,8 @@ BuildFieldReferenceExpr(Sema &S, Expr *BaseExpr, bool IsArrow, MemberType = S.Context.getQualifiedType(MemberType, Combined); } + S.UnusedPrivateFields.remove(Field); + ExprResult Base = S.PerformObjectMemberConversion(BaseExpr, SS.getScopeRep(), FoundDecl, Field); diff --git a/test/SemaCXX/warn-unused-member.cpp b/test/SemaCXX/warn-unused-member.cpp new file mode 100644 index 0000000000..16e1e22ee8 --- /dev/null +++ b/test/SemaCXX/warn-unused-member.cpp @@ -0,0 +1,193 @@ +// RUN: %clang_cc1 -fsyntax-only -Wunused-private-field -verify -std=c++11 %s + +class NotFullyDefined { + public: + NotFullyDefined(); + private: + int y; +}; + +class HasUndefinedNestedClass { + class Undefined; + int unused_; +}; + +class HasUndefinedPureVirtualDestructor { + virtual ~HasUndefinedPureVirtualDestructor() = 0; + int unused_; +}; + +class HasDefinedNestedClasses { + class DefinedHere {}; + class DefinedOutside; + int unused_; // expected-warning{{private field 'unused_' is not used}} +}; +class HasDefinedNestedClasses::DefinedOutside {}; + +class HasUndefinedFriendFunction { + friend void undefinedFriendFunction(); + int unused_; +}; + +class HasUndefinedFriendClass { + friend class NotFullyDefined; + friend class NotDefined; + int unused_; +}; + +class HasFriend { + friend class FriendClass; + friend void friendFunction(HasFriend f); + int unused_; // expected-warning{{private field 'unused_' is not used}} + int used_by_friend_class_; + int used_by_friend_function_; +}; + +class ClassWithTemplateFriend { + template friend class TemplateFriend; + int used_by_friend_; + int unused_; +}; + +template class TemplateFriend { +public: + TemplateFriend(ClassWithTemplateFriend my_friend) { + int var = my_friend.used_by_friend_; + } +}; + +class FriendClass { + HasFriend my_friend_; + void use() { + my_friend_.used_by_friend_class_ = 42; + } +}; + +void friendFunction(HasFriend my_friend) { + my_friend.used_by_friend_function_ = 42; +} + +class NonTrivialConstructor { + public: + NonTrivialConstructor() {} +}; + +class NonTrivialDestructor { + public: + ~NonTrivialDestructor() {} +}; + +class Trivial { + public: + Trivial() = default; + Trivial(int a) {} +}; + +int side_effect() { + return 42; +} + +class A { + public: + A() : primitive_type_(42), default_initializer_(), other_initializer_(42), + trivial_(), user_constructor_(42), + initialized_with_side_effect_(side_effect()) { + used_ = 42; + } + + A(int x, A* a) : pointer_(a) {} + + private: + int primitive_type_; // expected-warning{{private field 'primitive_type_' is not used}} + A* pointer_; // expected-warning{{private field 'pointer_' is not used}} + int no_initializer_; // expected-warning{{private field 'no_initializer_' is not used}} + int default_initializer_; // expected-warning{{private field 'default_initializer_' is not used}} + int other_initializer_; // expected-warning{{private field 'other_initializer_' is not used}} + int used_, unused_; // expected-warning{{private field 'unused_' is not used}} + int in_class_initializer_ = 42; // expected-warning{{private field 'in_class_initializer_' is not used}} + int in_class_initializer_with_side_effect_ = side_effect(); + Trivial trivial_initializer_ = Trivial(); + Trivial non_trivial_initializer_ = Trivial(42); + int initialized_with_side_effect_; + static int static_fields_are_ignored_; + + Trivial trivial_; // expected-warning{{private field 'trivial_' is not used}} + Trivial user_constructor_; + NonTrivialConstructor non_trivial_constructor_; + NonTrivialDestructor non_trivial_destructor_; +}; + +class EverythingUsed { + public: + EverythingUsed() : as_array_index_(0), var_(by_initializer_) { + var_ = sizeof(sizeof_); + int *use = &by_reference_; + int test[2]; + test[as_array_index_] = 42; + } + + template + void useStuff(T t) { + by_template_function_ = 42; + } + + private: + int var_; + int sizeof_; + int by_reference_; + int by_template_function_; + int as_array_index_; + int by_initializer_; +}; + +namespace mutual_friends { +// Undefined methods make mutual friends undefined. +class A { + int a; + friend class B; + void doSomethingToAOrB(); +}; +class B { + int b; + friend class A; +}; + +// Undefined friends do not make a mutual friend undefined. +class C { + int c; + void doSomethingElse() {} + friend class E; + friend class D; +}; +class D { + int d; // expected-warning{{private field 'd' is not used}} + friend class C; +}; + +// Undefined nested classes make mutual friends undefined. +class F { + int f; + class G; + friend class H; +}; +class H { + int h; + friend class F; +}; +} // namespace mutual_friends + +namespace anonymous_structs_unions { +class A { + private: + // FIXME: Look at the DeclContext for anonymous structs/unions. + union { + int *Aligner; + unsigned char Data[8]; + }; +}; +union S { + private: + int *Aligner; + unsigned char Data[8]; +}; +} // namespace anonymous_structs_unions -- 2.40.0