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?
> + 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.
> + @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.
> + - (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
