<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 Sep 27, 2012, at 2:12 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Comments comments comments:</div><br><div><div>On Sep 27, 2012, at 12:45 , Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">+static const ObjCIvarDecl *findPropertyBackingIvar(const ObjCPropertyDecl *PD,<br>+ ObjCInterfaceDecl *InterD,<br>+ ASTContext &Ctx) {<br>+ // Check for synthesized ivars.<br>+ ObjCIvarDecl *ID = PD->getPropertyIvarDecl();<br>+ if (ID)<br>+ return ID;<br>+<br>+ // Check for existing "_PropName".<br>+ ID = InterD->lookupInstanceVariable(PD->getDefaultSynthIvarName(Ctx));<br>+ if (ID)<br>+ return ID;<br>+<br>+ // Check for existing "PropName".<br>+ IdentifierInfo *PropIdent = PD->getIdentifier();<br>+ ID = InterD->lookupInstanceVariable(PropIdent);<br>+<br>+ return ID;<br>+}<br>+<br>+void DirectIvarAssignment::checkASTDecl(const ObjCImplementationDecl *D,<br>+ AnalysisManager& Mgr,<br>+ BugReporter &BR) const {<br>+ const ObjCInterfaceDecl *InterD = D->getClassInterface();<br>+<br>+<br>+ IvarToPropertyMapTy IvarToPropMap;<br>+<br>+ // Find all properties for this class.<br>+ for (ObjCInterfaceDecl::prop_iterator I = InterD->prop_begin(),<br>+ E = InterD->prop_end(); I != E; ++I) {<br>+ ObjCPropertyDecl *PD = *I;<br>+<br>+ // Find the corresponding IVar.<br>+ const ObjCIvarDecl *ID = findPropertyBackingIvar(PD,<br>+ const_cast<ObjCInterfaceDecl*>(InterD),<br>+ Mgr.getASTContext());<br></blockquote><div><br></div><div>Why the const_cast here? Why isn't the parameter just const?</div><div><br></div><br></div></div></blockquote><div><br></div>Parameter cannot be const. ObjCInterface <span style="font-family: Monaco; font-size: 11px; ">lookupInstanceVariable is not const. </span></div><div><font face="Monaco"><span style="font-size: 11px;"><br></span></font><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote type="cite">+ if (!ID)<br>+ continue;<br>+<br>+ // Store the IVar to property mapping.<br>+ IvarToPropMap[ID] = PD;<br>+ }<br>+<br>+ if (IvarToPropMap.empty())<br>+ return;<br>+<br>+ for (ObjCImplementationDecl::instmeth_iterator I = D->instmeth_begin(),<br>+ E = D->instmeth_end(); I != E; ++I) {<br>+<br>+ ObjCMethodDecl *M = *I;<br>+ AnalysisDeclContext *DCtx = Mgr.getAnalysisDeclContext(M);<br>+<br>+ if (M->getMethodFamily() == OMF_init ||<br>+ M->getMethodFamily() == OMF_dealloc ||<br>+ M->getSelector().getAsString().find("init") != StringRef::npos ||<br>+ M->getSelector().getAsString().find("Init") != StringRef::npos)<br>+ continue;<br></blockquote><div><br></div><div>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.</div></div></div></blockquote><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><br></div><div>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.</div><div><br></div><div><br></div><br><blockquote type="cite">+ const Stmt *Body = M->getBody();<br>+ if (!Body)<br>+ continue;<br></blockquote><div><br></div><div>I would go ahead and assert this: all methods in @implementation should have a body.</div><div><br></div><br><blockquote type="cite">+ MethodCrawler MC(IvarToPropMap, M->getCanonicalDecl(), InterD, BR, DCtx);<br>+ MC.VisitStmt(Body);<br>+ }<br>+}<br>+<br>+void DirectIvarAssignment::MethodCrawler::VisitBinaryOperator(<br>+ const BinaryOperator *BO) {<br>+ if (!BO->isAssignmentOp())<br>+ return;<br>+<br>+ const ObjCIvarRefExpr *IvarRef = dyn_cast<ObjCIvarRefExpr>(BO->getLHS());<br></blockquote><div><br></div><div>I'm not 100% sure but I think an IgnoreParenCasts is a good idea here.</div><div><br></div><br><blockquote type="cite">+ if (!IvarRef)<br>+ return;<br>+<br>+ if (const ObjCIvarDecl *D = IvarRef->getDecl()) {<br>+ IvarToPropertyMapTy::const_iterator I = IvarToPropMap.find(D);<br>+ if (I != IvarToPropMap.end()) {<br>+ const ObjCPropertyDecl *PD = I->second;<br>+<br>+ ObjCMethodDecl *GetterMethod =<br>+ InterfD->getInstanceMethod(PD->getGetterName());<br>+ ObjCMethodDecl *SetterMethod =<br>+ InterfD->getInstanceMethod(PD->getSetterName());<br>+<br>+ if (SetterMethod && SetterMethod->getCanonicalDecl() == MD)<br>+ return;<br>+<br>+ if (GetterMethod && GetterMethod->getCanonicalDecl() == MD)<br>+ return;<br>+<br>+<br>+ PathDiagnosticLocation IvarRefLocation =<br>+ PathDiagnosticLocation::createBegin(IvarRef,<br>+ BR.getSourceManager(), DCtx);<br></blockquote><div><br></div><div>Same comment as last time: createBegin is unnecessary here, just use the constructor that takes a Stmt to highlight the whole range.</div><div><br></div><br><blockquote type="cite">+ BR.EmitBasicReport(MD,<br>+ "Property access",<br>+ categories::CoreFoundationObjectiveC,<br>+ "Direct assignment to an instance variable backing a property; "<br>+ "use the setter instead", IvarRefLocation);<br>+ }<br>+ }<br>+}<br>+}<br>+<br>+void ento::registerDirectIvarAssignment(CheckerManager &mgr) {<br>+ mgr.registerChecker<DirectIvarAssignment>();<br>+}<br><br>Added: cfe/trunk/test/Analysis/objc-properties.m<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/objc-properties.m?rev=164790&view=auto">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/objc-properties.m?rev=164790&view=auto</a><br>==============================================================================<br>--- cfe/trunk/test/Analysis/objc-properties.m (added)<br>+++ cfe/trunk/test/Analysis/objc-properties.m Thu Sep 27 14:45:15 2012<br>@@ -0,0 +1,56 @@<br>+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.DirectIvarAssignment -fobjc-default-synthesize-properties -Wno-objc-root-class -verify -fblocks %s<br>+<br>+@interface MyClass;<br>+@end<br>+@interface TestProperty {<br>+ MyClass *_Z;<br>+ id _nonSynth;<br>+}<br>+<br>+ @property (assign, nonatomic) MyClass* A; // explicitely synthesized, not implemented, non-default ivar name<br>+<br>+ @property (assign) MyClass* X; // automatically synthesized, not implemented<br>+<br>+ @property (assign, nonatomic) MyClass* Y; // automatically synthesized, implemented<br>+<br>+ @property (assign, nonatomic) MyClass* Z; // non synthesized, implemented<br></blockquote><div><br></div><div>Z's <i>getter</i> is still synthesized, which means this test case isn't testing the case where the ObjCPropertyDecl doesn't have an associated ivar.</div></div></div></blockquote><div><br></div>maybe the comment is not correct.. the ivar is not synth here, _Z will be used, no?<br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><br></div><blockquote type="cite">+ @property (readonly) id nonSynth; // non synthesized, explicitly implemented to return ivar with expected name<br></blockquote><div><br></div><div>Ditto. You have to implement the getter for readonly and <i>both</i> getter and setter for readwrite in order to turn off ivar matching/synthesis.</div><div><br></div></div></div></blockquote>this one is readonly and I did implement the getter.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br><blockquote type="cite">+ - (id) initWithPtr:(MyClass*) value;<br>+ - (id) myInitWithPtr:(MyClass*) value;<br>+ - (void) someMethod: (MyClass*)In;<br>+@end<br>+<br>+@implementation TestProperty<br>+ @synthesize A = __A;<br>+ <br>+ - (id) initWithPtr: (MyClass*) value {<br>+ _Y = value; // no-warning<br>+ return self;<br>+ }<br>+<br>+ - (id) myInitWithPtr: (MyClass*) value {<br>+ _Y = value; // no-warning<br>+ return self;<br>+ }<br>+ <br>+ - (void) setY:(MyClass*) NewValue {<br>+ _Y = NewValue; // no-warning<br>+ }<br>+<br>+ - (void) setZ:(MyClass*) NewValue {<br>+ _Z = NewValue; // no-warning<br>+ }<br>+<br>+ - (id)nonSynth {<br>+ return _nonSynth;<br>+ }<br>+<br>+ - (void) someMethod: (MyClass*)In {<br>+ __A = In; // expected-warning {{Direct assignment to an instance variable backing a property; use the setter instead}}<br>+ _X = In; // expected-warning {{Direct assignment to an instance variable backing a property; use the setter instead}}<br>+ _Y = In; // expected-warning {{Direct assignment to an instance variable backing a property; use the setter instead}}<br>+ _Z = In; // expected-warning {{Direct assignment to an instance variable backing a property; use the setter instead}}<br>+ _nonSynth = 0; // expected-warning {{Direct assignment to an instance variable backing a property; use the setter instead}}<br>+ }<br>+@end<br>\ No newline at end of file<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></div></blockquote></div><br></body></html>