On Sep 28, 2012, at 6:51 PM, Jordan Rose <[email protected]> wrote:
> Comments (fewer this time): > > On Sep 28, 2012, at 17:20 , Anna Zaks <[email protected]> wrote: > >> >> /// Statement visitor, which walks the method body and flags the ivars >> /// referenced in it (either directly or via property). >> class MethodCrawler : public ConstStmtVisitor<MethodCrawler> { >> + const ObjCMethodDecl *EnclosingMethod; >> >> /// The set of Ivars which need to be invalidated. >> IvarSet &IVars; >> >> - /// Property setter/getter to ivar mapping. >> - MethToIvarMapTy &PropertyAccessorToIvarMap; >> + /// Flag is set as the result of a message send to another >> + /// invalidation method. >> + bool &CalledAnotherInvalidationMethod; >> + >> + /// Property setter to ivar mapping. >> + const MethToIvarMapTy &PropertySetterToIvarMap; >> + >> + /// Property getter to ivar mapping. >> + const MethToIvarMapTy &PropertyGetterToIvarMap; >> + >> + /// Property to ivar mapping. >> + const PropToIvarMapTy &PropertyToIvarMap; >> + >> + /// The invalidation method being currently processed. >> + const ObjCMethodDecl *InvalidationMethod; >> + >> + /// Peel off parents, casts, OpaqueValueExpr, and PseudoObjectExpr. >> + const Expr *peel(const Expr *E) const; > > Typo: "parens". > > > >> +const Expr *IvarInvalidationChecker::MethodCrawler::peel(const Expr *E) >> const { >> + E = E->IgnoreParenCasts(); >> + if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) >> + E = POE->getSyntacticForm()->IgnoreParenCasts(); >> + if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(E)) >> + E = OVE->getSourceExpr()->IgnoreParenCasts(); >> + return E; >> +} > > > > >> + >> +bool IvarInvalidationChecker::MethodCrawler::isZero(const Expr *E) const { >> + E = peel(E); >> + if (const IntegerLiteral *IL = dyn_cast<IntegerLiteral>(E)) >> + return IL->getValue() == 0; >> + >> + if (const CastExpr *ICE = dyn_cast<CastExpr>(E)) >> + return ICE->getCastKind() == CK_NullToPointer; >> + >> + return false; >> +} > > The more robust form of this is Expr::isNullPointerConstant. > isNullPointerConstant seems to do type checking, determining which type of null pointer we have. Why is it more robust to use here (all the typechecking has been done already)? > >> +void IvarInvalidationChecker::MethodCrawler::check(const Expr *E) { >> + E = peel(E); >> + >> + if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(E)) >> + E = OVE->getSourceExpr(); > > This should have been checked by peel already? leftover from internal refactor, thanks > > >> + if (const ObjCIvarRefExpr *IvarRef = dyn_cast<ObjCIvarRefExpr>(E)) { >> + checkObjCIvarRefExpr(IvarRef); >> + return; >> + } >> + >> + if (const ObjCPropertyRefExpr *PropRef = >> dyn_cast<ObjCPropertyRefExpr>(E)) { >> + checkObjCPropertyRefExpr(PropRef); >> + return; >> + } >> + >> + if (const ObjCMessageExpr *MsgExpr = dyn_cast<ObjCMessageExpr>(E)) { >> + checkObjCMessageExpr(MsgExpr); >> + return; >> + } >> +} >> + >> +void IvarInvalidationChecker::MethodCrawler::VisitBinaryOperator( >> + const BinaryOperator *BO) { >> + if (!BO->isAssignmentOp()) >> + return; > > You could probably be even more aggressive here and look for = specifically, > since += and such aren't valid on ObjC objects. yes, we should check only for '='. > >> + // Do we assign zero? >> + if (!isZero(BO->getRHS())) >> + return; >> + >> + // Check the variable we are assigning to. >> + check(BO->getLHS()); >> + >> + VisitStmt(BO); >> +} > > This VisitStmt is after two early returns. It seems like it should be first. yes. Thanks > > >> +void IvarInvalidationChecker::MethodCrawler::VisitObjCMessageExpr( >> + const ObjCMessageExpr *ME) { >> + const ObjCMethodDecl *MD = ME->getMethodDecl(); >> + const Expr *Receiver = ME->getInstanceReceiver(); >> + >> + // Stop if we are calling '[self invalidate]'. >> + if (Receiver && isInvalidationMethod(MD)) >> + if (const DeclRefExpr *RD = >> + dyn_cast<DeclRefExpr>(Receiver->IgnoreParenCasts())) { >> + if (RD->getDecl() == EnclosingMethod->getSelfDecl()) { >> + CalledAnotherInvalidationMethod = true; >> + return; >> + } >> + } >> + >> + // Check if we call a setter and set the property to 'nil'. >> + if (MD && (ME->getNumArgs() == 1) && isZero(ME->getArg(0))) { >> + MD = cast<ObjCMethodDecl>(MD->getCanonicalDecl()); >> + MethToIvarMapTy::const_iterator IvI = PropertySetterToIvarMap.find(MD); >> + if (IvI != PropertySetterToIvarMap.end()) { >> + markInvalidated(IvI->second); >> + return; >> + } >> + } >> + >> + // Check if we call the 'invalidation' routine on the ivar. >> + if (Receiver) { >> + InvalidationMethod = MD; >> + check(Receiver->IgnoreParenCasts()); >> + InvalidationMethod = 0; > > Passing state around like this does not make me happy. It does not make me happy either.:) It does not make me more happy to pass this as a parameter through a bunch of calls. > In particular, [[a foo] bar] will not work because the check() method calls > back to VisitObjCMessageExpr. It does not. > Then again, no one will write an invalidation method like this. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
