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

[Jordan Rose]

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.


> +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?


> +  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.

> +  // 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.


> +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. In particular, [[a foo] bar] will not work because the check() method calls back to VisitObjCMessageExpr. Then again, no one will write an invalidation method like this.