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

[Alexander Droste via cfe-commits]

Alexander_Droste added inline comments.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43
@@ +42,3 @@
+private:
+  const std::unique_ptr<MPICheckerPathSensitive> CheckerSens;
+
----------------
zaks.anna wrote:
> Alexander_Droste wrote:
> > zaks.anna wrote:
> > > I'd stress "path" instead of "sensitive" in the name. Also, this indirection is redundant if you remove the AST checks.
> > If sufficient, I would rename `MPICheckerPathSensitive` to `MPICheckerPath` then.
> > Do you mind the indirection?
> I indirection buying us anything? I think it makes the code more difficult to follow,
I think, I like the entry point class being separated from the class which does the actual checking. But the necessity for `dynamicInit()` maybe outweighs that. I can collapse the two.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:74
@@ +73,3 @@
+      // A wait has no matching nonblocking call.
+      BugReporter.reportUnmatchedWait(PreCallEvent, ReqRegion, ExplNode);
+    }
----------------
zaks.anna wrote:
> Alexander_Droste wrote:
> > zaks.anna wrote:
> > > This is called in a loop. Should you break once the first error is reported?
> > > 
> > > Also, as before you should use the CheckerContext API to produce a node on which the error should be reported.
> > > 
> > > 
> > I don't think break should be called after the first error is reported. Each memory region
> > represents a different request, why this should be rated as multiple errors. 
> You might be right, I am not 100% sure what's more user-friendly..  Presumably the fixes are different in each case? Could you add a test case where multiple errors on the same call site are reported? (Ups can use expected-warning at +1 to report multiple warnings on the same line.)
This is comparable to calling free on different elements, for which memory was never allocated. So each report informs about a distinct missing nonblocking call. I'll give `expected-warning at +1` a try.


http://reviews.llvm.org/D12761