On Sep 27, 2012, at 2:12 PM, Jordan Rose <[email protected]> wrote:
> Comments comments comments: > > On Sep 27, 2012, at 12:45 , Anna Zaks <[email protected]> wrote: > >> +static const ObjCIvarDecl *findPropertyBackingIvar(const ObjCPropertyDecl >> *PD, >> + ObjCInterfaceDecl >> *InterD, >> + ASTContext &Ctx) { >> + // Check for synthesized ivars. >> + ObjCIvarDecl *ID = PD->getPropertyIvarDecl(); >> + if (ID) >> + return ID; >> + >> + // Check for existing "_PropName". >> + ID = InterD->lookupInstanceVariable(PD->getDefaultSynthIvarName(Ctx)); >> + if (ID) >> + return ID; >> + >> + // Check for existing "PropName". >> + IdentifierInfo *PropIdent = PD->getIdentifier(); >> + ID = InterD->lookupInstanceVariable(PropIdent); >> + >> + return ID; >> +} >> + >> +void DirectIvarAssignment::checkASTDecl(const ObjCImplementationDecl *D, >> + AnalysisManager& Mgr, >> + BugReporter &BR) const { >> + const ObjCInterfaceDecl *InterD = D->getClassInterface(); >> + >> + >> + IvarToPropertyMapTy IvarToPropMap; >> + >> + // Find all properties for this class. >> + for (ObjCInterfaceDecl::prop_iterator I = InterD->prop_begin(), >> + E = InterD->prop_end(); I != E; ++I) { >> + ObjCPropertyDecl *PD = *I; >> + >> + // Find the corresponding IVar. >> + const ObjCIvarDecl *ID = findPropertyBackingIvar(PD, >> + const_cast<ObjCInterfaceDecl*>(InterD), >> + Mgr.getASTContext()); > > Why the const_cast here? Why isn't the parameter just const? > > Parameter cannot be const. ObjCInterface lookupInstanceVariable is not const. >> + if (!ID) >> + continue; >> + >> + // Store the IVar to property mapping. >> + IvarToPropMap[ID] = PD; >> + } >> + >> + if (IvarToPropMap.empty()) >> + return; >> + >> + for (ObjCImplementationDecl::instmeth_iterator I = D->instmeth_begin(), >> + E = D->instmeth_end(); I != E; ++I) { >> + >> + ObjCMethodDecl *M = *I; >> + AnalysisDeclContext *DCtx = Mgr.getAnalysisDeclContext(M); >> + >> + if (M->getMethodFamily() == OMF_init || >> + M->getMethodFamily() == OMF_dealloc || >> + M->getSelector().getAsString().find("init") != StringRef::npos || >> + M->getSelector().getAsString().find("Init") != StringRef::npos) >> + continue; > > Converting the selector to a string isn't particularly cheap. The "best" > thing to do here would be to check each piece of the selector, but if you do > want to use getAsString() for simplicity/ease of maintenance it'd be nice to > do it just once. > > Also, I'd appreciate a comment to say what the string-search is trying to > solve, because my initial thought was "but any method starting with the word > 'init' is automatically marked as OMF_init" before I figured it out. > > > >> + const Stmt *Body = M->getBody(); >> + if (!Body) >> + continue; > > I would go ahead and assert this: all methods in @implementation should have > a body. > > >> + MethodCrawler MC(IvarToPropMap, M->getCanonicalDecl(), InterD, BR, >> DCtx); >> + MC.VisitStmt(Body); >> + } >> +} >> + >> +void DirectIvarAssignment::MethodCrawler::VisitBinaryOperator( >> + const BinaryOperator >> *BO) { >> + if (!BO->isAssignmentOp()) >> + return; >> + >> + const ObjCIvarRefExpr *IvarRef = dyn_cast<ObjCIvarRefExpr>(BO->getLHS()); > > I'm not 100% sure but I think an IgnoreParenCasts is a good idea here. > > >> + if (!IvarRef) >> + return; >> + >> + if (const ObjCIvarDecl *D = IvarRef->getDecl()) { >> + IvarToPropertyMapTy::const_iterator I = IvarToPropMap.find(D); >> + if (I != IvarToPropMap.end()) { >> + const ObjCPropertyDecl *PD = I->second; >> + >> + ObjCMethodDecl *GetterMethod = >> + InterfD->getInstanceMethod(PD->getGetterName()); >> + ObjCMethodDecl *SetterMethod = >> + InterfD->getInstanceMethod(PD->getSetterName()); >> + >> + if (SetterMethod && SetterMethod->getCanonicalDecl() == MD) >> + return; >> + >> + if (GetterMethod && GetterMethod->getCanonicalDecl() == MD) >> + return; >> + >> + >> + PathDiagnosticLocation IvarRefLocation = >> + PathDiagnosticLocation::createBegin(IvarRef, >> + BR.getSourceManager(), DCtx); > > Same comment as last time: createBegin is unnecessary here, just use the > constructor that takes a Stmt to highlight the whole range. > > >> + BR.EmitBasicReport(MD, >> + "Property access", >> + categories::CoreFoundationObjectiveC, >> + "Direct assignment to an instance variable backing a property; " >> + "use the setter instead", IvarRefLocation); >> + } >> + } >> +} >> +} >> + >> +void ento::registerDirectIvarAssignment(CheckerManager &mgr) { >> + mgr.registerChecker<DirectIvarAssignment>(); >> +} >> >> Added: cfe/trunk/test/Analysis/objc-properties.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/objc-properties.m?rev=164790&view=auto >> ============================================================================== >> --- cfe/trunk/test/Analysis/objc-properties.m (added) >> +++ cfe/trunk/test/Analysis/objc-properties.m Thu Sep 27 14:45:15 2012 >> @@ -0,0 +1,56 @@ >> +// RUN: %clang_cc1 -analyze >> -analyzer-checker=alpha.osx.cocoa.DirectIvarAssignment >> -fobjc-default-synthesize-properties -Wno-objc-root-class -verify -fblocks %s >> + >> +@interface MyClass; >> +@end >> +@interface TestProperty { >> + MyClass *_Z; >> + id _nonSynth; >> +} >> + >> + @property (assign, nonatomic) MyClass* A; // explicitely synthesized, not >> implemented, non-default ivar name >> + >> + @property (assign) MyClass* X; // automatically synthesized, not >> implemented >> + >> + @property (assign, nonatomic) MyClass* Y; // automatically synthesized, >> implemented >> + >> + @property (assign, nonatomic) MyClass* Z; // non synthesized, implemented > > Z's getter is still synthesized, which means this test case isn't testing the > case where the ObjCPropertyDecl doesn't have an associated ivar. maybe the comment is not correct.. the ivar is not synth here, _Z will be used, no? > >> + @property (readonly) id nonSynth; // non synthesized, explicitly >> implemented to return ivar with expected name > > Ditto. You have to implement the getter for readonly and both getter and > setter for readwrite in order to turn off ivar matching/synthesis. > this one is readonly and I did implement the getter. > >> + - (id) initWithPtr:(MyClass*) value; >> + - (id) myInitWithPtr:(MyClass*) value; >> + - (void) someMethod: (MyClass*)In; >> +@end >> + >> +@implementation TestProperty >> + @synthesize A = __A; >> + >> + - (id) initWithPtr: (MyClass*) value { >> + _Y = value; // no-warning >> + return self; >> + } >> + >> + - (id) myInitWithPtr: (MyClass*) value { >> + _Y = value; // no-warning >> + return self; >> + } >> + >> + - (void) setY:(MyClass*) NewValue { >> + _Y = NewValue; // no-warning >> + } >> + >> + - (void) setZ:(MyClass*) NewValue { >> + _Z = NewValue; // no-warning >> + } >> + >> + - (id)nonSynth { >> + return _nonSynth; >> + } >> + >> + - (void) someMethod: (MyClass*)In { >> + __A = In; // expected-warning {{Direct assignment to an instance >> variable backing a property; use the setter instead}} >> + _X = In; // expected-warning {{Direct assignment to an instance >> variable backing a property; use the setter instead}} >> + _Y = In; // expected-warning {{Direct assignment to an instance >> variable backing a property; use the setter instead}} >> + _Z = In; // expected-warning {{Direct assignment to an instance >> variable backing a property; use the setter instead}} >> + _nonSynth = 0; // expected-warning {{Direct assignment to an instance >> variable backing a property; use the setter instead}} >> + } >> +@end >> \ No newline at end of file >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
