[cfe-commits] r164868 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp test/Analysis/objc_invalidation.m

[Anna Zaks]

On Sep 28, 2012, at 6:51 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Comments (fewer this time):
> 
> On Sep 28, 2012, at 17:20 , Anna Zaks <ganna at apple.com> 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.