On Fri, Jan 3, 2014 at 1:32 PM, Argyrios Kyrtzidis <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
