On Jan 3, 2014, at 10:40 AM, Aaron Ballman <[email protected]> wrote:
> 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. In r198442, thanks for reviewing! > > ~Aaron
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
