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