r198432 - [objc] Refactor and improve functionality for the -Wunused-property-ivar warning.
Aaron Ballman
aaron at aaronballman.com
Fri Jan 3 10:40:39 PST 2014
On Fri, Jan 3, 2014 at 1:32 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> Author: akirtzidis
> Date: Fri Jan 3 12:32:18 2014
> New Revision: 198432
>
> URL: http://llvm.org/viewvc/llvm-project?rev=198432&view=rev
> Log:
> [objc] Refactor and improve functionality for the -Wunused-property-ivar warning.
>
> - Remove the additions to ObjCMethodDecl & ObjCIVarDecl that were getting de/serialized and consolidate
> all functionality for the checking for this warning in Sema::DiagnoseUnusedBackingIvarInAccessor
> - Don't check immediately after the method body is finished, check when the @implementation is finished.
> This is so we can see if the ivar was referenced by any other method, even if the method was defined after the accessor.
> - Don't silence the warning if any method is called from the accessor silence it if the accessor delegates to another method via self.
>
> rdar://15727325
>
> Modified:
> cfe/trunk/include/clang/AST/DeclObjC.h
> cfe/trunk/include/clang/Sema/Sema.h
> cfe/trunk/lib/AST/ASTDumper.cpp
> cfe/trunk/lib/AST/ASTImporter.cpp
> cfe/trunk/lib/AST/DeclObjC.cpp
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> cfe/trunk/lib/Sema/SemaExpr.cpp
> cfe/trunk/lib/Sema/SemaExprObjC.cpp
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
> cfe/trunk/test/SemaObjC/unused-backing-ivar-warning.m
>
> Modified: cfe/trunk/include/clang/AST/DeclObjC.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
> +++ cfe/trunk/include/clang/AST/DeclObjC.h Fri Jan 3 12:32:18 2014
> @@ -161,9 +161,6 @@ private:
>
> /// \brief Indicates if the method was a definition but its body was skipped.
> unsigned HasSkippedBody : 1;
> -
> - /// \brief True if method body makes a method call.
> - unsigned MethodCallsMethod : 1;
>
> // Result type of this method.
> QualType MethodDeclType;
> @@ -245,7 +242,7 @@ private:
> DeclImplementation(impControl), objcDeclQualifier(OBJC_TQ_None),
> RelatedResultType(HasRelatedResultType),
> SelLocsKind(SelLoc_StandardNoSpace), IsOverriding(0), HasSkippedBody(0),
> - MethodCallsMethod(0), MethodDeclType(T), ResultTInfo(ResultTInfo),
> + MethodDeclType(T), ResultTInfo(ResultTInfo),
> ParamsAndSelLocs(0), NumParams(0),
> DeclEndLoc(endLoc), Body(), SelfDecl(0), CmdDecl(0) {
> setImplicit(isImplicitlyDeclared);
> @@ -439,9 +436,6 @@ public:
> bool hasSkippedBody() const { return HasSkippedBody; }
> void setHasSkippedBody(bool Skipped = true) { HasSkippedBody = Skipped; }
>
> - bool getMethodCallsMethod() const { return MethodCallsMethod; }
> - void setMethodCallsMethod(bool val = true) { MethodCallsMethod = val; }
> -
> /// \brief Returns the property associated with this method's selector.
> ///
> /// Note that even if this particular method is not marked as a property
> @@ -1326,12 +1320,10 @@ private:
> ObjCIvarDecl(ObjCContainerDecl *DC, SourceLocation StartLoc,
> SourceLocation IdLoc, IdentifierInfo *Id,
> QualType T, TypeSourceInfo *TInfo, AccessControl ac, Expr *BW,
> - bool synthesized,
> - bool backingIvarReferencedInAccessor)
> + bool synthesized)
> : FieldDecl(ObjCIvar, DC, StartLoc, IdLoc, Id, T, TInfo, BW,
> /*Mutable=*/false, /*HasInit=*/ICIS_NoInit),
> - NextIvar(0), DeclAccess(ac), Synthesized(synthesized),
> - BackingIvarReferencedInAccessor(backingIvarReferencedInAccessor) {}
> + NextIvar(0), DeclAccess(ac), Synthesized(synthesized) {}
>
> public:
> static ObjCIvarDecl *Create(ASTContext &C, ObjCContainerDecl *DC,
> @@ -1339,8 +1331,7 @@ public:
> IdentifierInfo *Id, QualType T,
> TypeSourceInfo *TInfo,
> AccessControl ac, Expr *BW = NULL,
> - bool synthesized=false,
> - bool backingIvarReferencedInAccessor=false);
> + bool synthesized=false);
>
> static ObjCIvarDecl *CreateDeserialized(ASTContext &C, unsigned ID);
>
> @@ -1362,13 +1353,6 @@ public:
> return DeclAccess == None ? Protected : AccessControl(DeclAccess);
> }
>
> - void setBackingIvarReferencedInAccessor(bool val) {
> - BackingIvarReferencedInAccessor = val;
> - }
> - bool getBackingIvarReferencedInAccessor() const {
> - return BackingIvarReferencedInAccessor;
> - }
> -
> void setSynthesize(bool synth) { Synthesized = synth; }
> bool getSynthesize() const { return Synthesized; }
>
> @@ -1383,7 +1367,6 @@ private:
> // NOTE: VC++ treats enums as signed, avoid using the AccessControl enum
> unsigned DeclAccess : 3;
> unsigned Synthesized : 1;
> - unsigned BackingIvarReferencedInAccessor : 1;
> };
>
>
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Fri Jan 3 12:32:18 2014
> @@ -833,6 +833,7 @@ public:
>
> /// Private Helper predicate to check for 'self'.
> bool isSelfExpr(Expr *RExpr);
> + bool isSelfExpr(Expr *RExpr, const ObjCMethodDecl *Method);
>
> /// \brief Cause the active diagnostic on the DiagosticsEngine to be
> /// emitted. This is closely coupled to the SemaDiagnosticBuilder class and
> @@ -2639,7 +2640,8 @@ public:
>
> /// DiagnoseUnusedBackingIvarInAccessor - Issue an 'unused' warning if ivar which
> /// backs the property is not used in the property's accessor.
> - void DiagnoseUnusedBackingIvarInAccessor(Scope *S);
> + void DiagnoseUnusedBackingIvarInAccessor(Scope *S,
> + const ObjCImplementationDecl *ImplD);
>
> /// GetIvarBackingPropertyAccessor - If method is a property setter/getter and
> /// it property has a backing ivar, returns this ivar; otherwise, returns NULL.
>
> Modified: cfe/trunk/lib/AST/ASTDumper.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/ASTDumper.cpp (original)
> +++ cfe/trunk/lib/AST/ASTDumper.cpp Fri Jan 3 12:32:18 2014
> @@ -1241,8 +1241,6 @@ void ASTDumper::VisitObjCIvarDecl(const
> dumpType(D->getType());
> if (D->getSynthesize())
> OS << " synthesize";
> - if (D->getBackingIvarReferencedInAccessor())
> - OS << " BackingIvarReferencedInAccessor";
>
> switch (D->getAccessControl()) {
> case ObjCIvarDecl::None:
>
> Modified: cfe/trunk/lib/AST/ASTImporter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/ASTImporter.cpp (original)
> +++ cfe/trunk/lib/AST/ASTImporter.cpp Fri Jan 3 12:32:18 2014
> @@ -3016,8 +3016,7 @@ Decl *ASTNodeImporter::VisitObjCIvarDecl
> Importer.Import(D->getInnerLocStart()),
> Loc, Name.getAsIdentifierInfo(),
> T, TInfo, D->getAccessControl(),
> - BitWidth, D->getSynthesize(),
> - D->getBackingIvarReferencedInAccessor());
> + BitWidth, D->getSynthesize());
> ToIvar->setLexicalDeclContext(LexicalDC);
> Importer.Imported(D, ToIvar);
> LexicalDC->addDeclInternal(ToIvar);
>
> Modified: cfe/trunk/lib/AST/DeclObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/DeclObjC.cpp (original)
> +++ cfe/trunk/lib/AST/DeclObjC.cpp Fri Jan 3 12:32:18 2014
> @@ -1440,8 +1440,7 @@ ObjCIvarDecl *ObjCIvarDecl::Create(ASTCo
> SourceLocation IdLoc, IdentifierInfo *Id,
> QualType T, TypeSourceInfo *TInfo,
> AccessControl ac, Expr *BW,
> - bool synthesized,
> - bool backingIvarReferencedInAccessor) {
> + bool synthesized) {
> if (DC) {
> // Ivar's can only appear in interfaces, implementations (via synthesized
> // properties), and class extensions (via direct declaration, or synthesized
> @@ -1469,13 +1468,12 @@ ObjCIvarDecl *ObjCIvarDecl::Create(ASTCo
> }
>
> return new (C, DC) ObjCIvarDecl(DC, StartLoc, IdLoc, Id, T, TInfo, ac, BW,
> - synthesized, backingIvarReferencedInAccessor);
> + synthesized);
> }
>
> ObjCIvarDecl *ObjCIvarDecl::CreateDeserialized(ASTContext &C, unsigned ID) {
> return new (C, ID) ObjCIvarDecl(0, SourceLocation(), SourceLocation(), 0,
> - QualType(), 0, ObjCIvarDecl::None, 0, false,
> - false);
> + QualType(), 0, ObjCIvarDecl::None, 0, false);
> }
>
> const ObjCInterfaceDecl *ObjCIvarDecl::getContainingInterface() const {
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Jan 3 12:32:18 2014
> @@ -1392,7 +1392,6 @@ void Sema::ActOnPopScope(SourceLocation
> // Remove this name from our lexical scope.
> IdResolver.RemoveDecl(D);
> }
> - DiagnoseUnusedBackingIvarInAccessor(S);
> }
>
> void Sema::ActOnStartFunctionDeclarator() {
>
> Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=198432&r1=198431&r2=198432&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Fri Jan 3 12:32:18 2014
> @@ -15,6 +15,7 @@
> #include "clang/AST/ASTConsumer.h"
> #include "clang/AST/ASTContext.h"
> #include "clang/AST/ASTMutationListener.h"
> +#include "clang/AST/DataRecursiveASTVisitor.h"
> #include "clang/AST/DeclObjC.h"
> #include "clang/AST/Expr.h"
> #include "clang/AST/ExprObjC.h"
> @@ -2693,6 +2694,7 @@ Decl *Sema::ActOnAtEnd(Scope *S, SourceR
> ImplMethodsVsClassMethods(S, IC, IDecl);
> AtomicPropertySetterGetterRules(IC, IDecl);
> DiagnoseOwningPropertyGetterSynthesis(IC);
> + DiagnoseUnusedBackingIvarInAccessor(S, IC);
> if (IDecl->hasDesignatedInitializers())
> DiagnoseMissingDesignatedInitOverrides(IC, IDecl);
>
> @@ -3494,39 +3496,83 @@ Sema::GetIvarBackingPropertyAccessor(con
> const ObjCInterfaceDecl *IDecl = Method->getClassInterface();
> if (!IDecl)
> return 0;
> - Method = IDecl->lookupMethod(Method->getSelector(), true);
> + Method = IDecl->lookupMethod(Method->getSelector(), /*isInstance=*/true,
> + /*shallowCategoryLookup=*/false,
> + /*followSuper=*/false);
> if (!Method || !Method->isPropertyAccessor())
> return 0;
> - if ((PDecl = Method->findPropertyDecl())) {
> - if (!PDecl->getDeclContext())
> - return 0;
> - // Make sure property belongs to accessor's class and not to
> - // one of its super classes.
> - if (const ObjCInterfaceDecl *CID =
> - dyn_cast<ObjCInterfaceDecl>(PDecl->getDeclContext()))
> - if (CID != IDecl)
> - return 0;
> + if ((PDecl = Method->findPropertyDecl()))
> return PDecl->getPropertyIvarDecl();
> - }
> +
> return 0;
> }
>
> -void Sema::DiagnoseUnusedBackingIvarInAccessor(Scope *S) {
> - if (S->hasUnrecoverableErrorOccurred() || !S->isInObjcMethodOuterScope())
> - return;
> -
> - const ObjCMethodDecl *CurMethod = getCurMethodDecl();
> - if (!CurMethod)
> +namespace {
> + /// Used by Sema::DiagnoseUnusedBackingIvarInAccessor to check if a property
> + /// accessor references the backing ivar.
> + class UnusedBackingIvarChecker : public DataRecursiveASTVisitor<UnusedBackingIvarChecker> {
> + public:
> + Sema &S;
> + const ObjCMethodDecl *Method;
> + const ObjCIvarDecl *IvarD;
> + bool AccessedIvar;
> + bool InvokedSelfMethod;
> +
> + UnusedBackingIvarChecker(Sema &S, const ObjCMethodDecl *Method,
> + const ObjCIvarDecl *IvarD)
> + : S(S), Method(Method), IvarD(IvarD),
> + AccessedIvar(false), InvokedSelfMethod(false) {
> + assert(IvarD);
> + }
> +
> + bool VisitObjCIvarRefExpr(ObjCIvarRefExpr *E) {
> + if (E->getDecl() == IvarD) {
> + AccessedIvar = true;
> + return false;
> + }
> + return true;
> + }
> +
> + bool VisitObjCMessageExpr(ObjCMessageExpr *E) {
> + if (E->getReceiverKind() == ObjCMessageExpr::Instance &&
> + S.isSelfExpr(E->getInstanceReceiver(), Method)) {
> + InvokedSelfMethod = true;
> + }
> + return true;
> + }
> + };
> +}
> +
> +void Sema::DiagnoseUnusedBackingIvarInAccessor(Scope *S,
> + const ObjCImplementationDecl *ImplD) {
> + if (S->hasUnrecoverableErrorOccurred())
> return;
> - const ObjCPropertyDecl *PDecl;
> - const ObjCIvarDecl *IV = GetIvarBackingPropertyAccessor(CurMethod, PDecl);
> - if (IV && !IV->getBackingIvarReferencedInAccessor()) {
> +
> + for (ObjCImplementationDecl::instmeth_iterator
> + MI = ImplD->instmeth_begin(),
> + ME = ImplD->instmeth_end(); MI != ME; ++MI) {
> + const ObjCMethodDecl *CurMethod = *MI;
> + unsigned DIAG = diag::warn_unused_property_backing_ivar;
> + SourceLocation Loc = CurMethod->getLocation();
> + if (Diags.getDiagnosticLevel(DIAG, Loc) == DiagnosticsEngine::Ignored)
> + continue;
> +
> + const ObjCPropertyDecl *PDecl;
> + const ObjCIvarDecl *IV = GetIvarBackingPropertyAccessor(CurMethod, PDecl);
> + if (!IV)
> + continue;
> +
> + UnusedBackingIvarChecker Checker(*this, CurMethod, IV);
> + Checker.TraverseStmt(CurMethod->getBody());
> + if (Checker.AccessedIvar)
> + continue;
> +
> // Do not issue this warning if backing ivar is used somewhere and accessor
> - // implementation makes a call to a method. This is to prevent false positive in
> - // some corner cases.
> - if (!IV->isReferenced() || !CurMethod->getMethodCallsMethod()) {
> - Diag(getCurMethodDecl()->getLocation(), diag::warn_unused_property_backing_ivar)
> - << IV->getDeclName();
> + // implementation makes a self call. This is to prevent false positive in
> + // cases where the ivar is accessed by another method that the accessor
> + // delegates to.
> + if (!IV->isReferenced() || !Checker.InvokedSelfMethod) {
> + Diag(Loc, DIAG) << IV->getDeclName();
Minor nitpick: I don't think you need to call getDeclName here --
ObjCIvarDecl is a FieldDecl, which ultimately is a NamedDecl, which
the diagnostics engine understands how to print. You can just pass in
IV.
~Aaron
More information about the cfe-commits
mailing list