[PATCH] D12761: MPI-Checker patch for Clang Static Analyzer

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Some comments inline. I still have to do an overall pass over this checker, but it looks much better than the first version!


================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:47
@@ +46,3 @@
+    BReporter->reportDoubleNonblocking(PreCallEvent, *Req, MR, ExplNode);
+    Ctx.addTransition(ErrorNode->getState(), ErrorNode);
+  }
----------------
Correct, it should be ErrorNode. Moreover, if you do not modify the State and use the state from the predecessor node, which is the case here, you do not need to pass a State to it. You do not need to pass the tag either in his case - a page generated for your checker will be used. By default the CheckerContext will use the state of the predecessor node and that default tag. (If you want to pass your tag, it's fine.)

You do need to pass the state if you modify the state as in the next example.

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:84
@@ +83,3 @@
+      if (!ErrorNode) {
+        ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+        State = ErrorNode->getState();
----------------
Here you do need to pass State.

================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:135
@@ +134,3 @@
+
+void MPIChecker::checkMissingWaitsGlobals(ExplodedGraph &G) const {
+
----------------
I do not think I've seen this code before. If you add new functionality, please submit it in a new commit. Otherwise, there is a high chance it will go unnoticed. 

Are there examples of the new warnings it caught? 

checkEndAnalysis is rarely used by checkers, so you might not need it here. Specifically, you should be notified by the checkDeadSymbols before a symbol dies even if it dies because it's the end of path.


http://reviews.llvm.org/D12761