Add PostStmt callback for ArraySubscriptExpr

[a.sidorin]

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

28 ↗

(On Diff #72781)

I'd like to see Stmt-related options together (PreStmt<Type1>, PostStmt<Type1>, PreStmt<Type2>, PostStmt<Type2>). Not sure, however. Anna, Artem, what do you think? Is there any style recommendation for that?

lib/StaticAnalyzer/Core/ExprEngine.cpp

1990 ↗

(On Diff #72781)

You should use a temporary node set for PreStmt and then use Dst for PostStmt. Please see an example in ExprEngine.cpp:1213.

[a.sidorin]

lib/StaticAnalyzer/Core/ExprEngine.cpp

1980 ↗

(On Diff #72811)

auto *Node

[a.sidorin]

Looks good. Anna/Artem?

[NoQ]

So we did run checkers for PreStmt but forgot PostStmt? Nice! Thanks for noticing and fixing.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

28–30 ↗

(On Diff #72815)

I agree that having one place to change stuff is better than having two places to change stuff, so i'd also prefer pre-post-pre-pos-...-pre-post. In the long term, when this list becomes very long, i'd still suggest we only make PreStmt<Stmt> and PostStmt<Stmt> and construct some dynamic switch in the implementation.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1980 ↗

(On Diff #72815)

Indent seems to have moved.

[zaks.anna]

Looks good overall.

Please, include a more elaborate commit message, mentioning that you are adding a new callback.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1971 ↗

(On Diff #72815)

Let's keep the prefix of the old name for "checkerPreStmt" as it is more explicit - those are the states we get by running the checkers registered for preStmt. This is also more consistent with naming used elsewhere.

(Not sure what to do about capitalization though. Looks like it's already inconsistent in this file. The LLVM coding style prefers lower letters, but this part of the analyzer has been written before that went into effect.)

[a.sidorin]

Seems like all comments were addressed. Thank you!

[zaks.anna]

Thank you!

Do you have commit access or should we commit?

[jan]

Please go ahead and commit. I do not have access.
Thanks

cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>,
-                                             check::PostStmt<CastExpr>> {
+                                             check::PostStmt<CastExpr>,
+                                             check::PreStmt<ArraySubscriptExpr>,
+                                             check::PostStmt<ArraySubscriptExpr>> {
   bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
     AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
     return Opts.getBooleanOption("*", false, this) ||
         Opts.getBooleanOption(CallbackName, false, this);
   }
 
 public:
   void checkPreStmt(const CastExpr *CE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PreStmtCastExpr"))
       llvm::errs() << "PreStmt<CastExpr> (Kind : " << CE->getCastKindName()
                    << ")\n";
   }
 
   void checkPostStmt(const CastExpr *CE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtCastExpr"))
       llvm::errs() << "PostStmt<CastExpr> (Kind : " << CE->getCastKindName()
                    << ")\n";
   }
+
+  void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr"))
+      llvm::errs() << "PreStmt<ArraySubscriptExpr>\n";
+  }
+
+  void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
+      llvm::errs() << "PostStmt<ArraySubscriptExpr>\n";
+  }
 };
 }
 
 //===----------------------------------------------------------------------===//
 // Registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerAnalysisOrderChecker(CheckerManager &mgr) {
   mgr.registerChecker<AnalysisOrderChecker>();
 }

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

 /// VisitArraySubscriptExpr - Transfer function for array accesses
 void ExprEngine::VisitLvalArraySubscriptExpr(const ArraySubscriptExpr *A,
                                              ExplodedNode *Pred,
                                              ExplodedNodeSet &Dst){
 
   const Expr *Base = A->getBase()->IgnoreParens();
   const Expr *Idx  = A->getIdx()->IgnoreParens();
 
-  ExplodedNodeSet checkerPreStmt;
-  getCheckerManager().runCheckersForPreStmt(checkerPreStmt, Pred, A, *this);
+  ExplodedNodeSet CheckerPreStmt;
+  getCheckerManager().runCheckersForPreStmt(CheckerPreStmt, Pred, A, *this);
 
-  StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
+  ExplodedNodeSet EvalSet;
+  StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
   assert(A->isGLValue() ||
           (!AMgr.getLangOpts().CPlusPlus &&
            A->getType().isCForbiddenLValueType()));
 
-  for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
-                                 ei = checkerPreStmt.end(); it != ei; ++it) {
-    const LocationContext *LCtx = (*it)->getLocationContext();
-    ProgramStateRef state = (*it)->getState();
+  for (auto *Node : CheckerPreStmt) {
+    const LocationContext *LCtx = Node->getLocationContext();
+    ProgramStateRef state = Node->getState();
     SVal V = state->getLValue(A->getType(),
                               state->getSVal(Idx, LCtx),
                               state->getSVal(Base, LCtx));
-    Bldr.generateNode(A, *it, state->BindExpr(A, LCtx, V), nullptr,
+    Bldr.generateNode(A, Node, state->BindExpr(A, LCtx, V), nullptr,
                       ProgramPoint::PostLValueKind);
   }
+
+  getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
 }
 
 /// VisitMemberExpr - Transfer function for member expressions.
 void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
                                  ExplodedNodeSet &Dst) {
 
   // FIXME: Prechecks eventually go in ::Visit().
   ExplodedNodeSet CheckedSet;