r198432 - [objc] Refactor and improve functionality for the -Wunused-property-ivar warning.
Argyrios Kyrtzidis
akyrtzi at gmail.com
Fri Jan 3 11:44:43 PST 2014
On Jan 3, 2014, at 10:40 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.
In r198442, thanks for reviewing!
>
> ~Aaron
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140103/29cb8fb7/attachment.html>
More information about the cfe-commits
mailing list