[cfe-commits] r164790 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp test/Analysis/objc-properties.m
Anna Zaks
ganna at apple.com
Thu Sep 27 14:45:06 PDT 2012
On Sep 27, 2012, at 2:12 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> Comments comments comments:
>
> On Sep 27, 2012, at 12:45 , Anna Zaks <ganna at apple.com> 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
>> +
>> + at interface MyClass;
>> + at end
>> + at 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;
>> + at end
>> +
>> + at 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}}
>> + }
>> + at end
>> \ No newline at end of file
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120927/090bab16/attachment.html>
More information about the cfe-commits
mailing list