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

Anna Zaks via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 3 14:58:27 PST 2016


zaks.anna added inline comments.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74,3 @@
 
+def MPI : Package<"mpi">;
+
----------------
Alexander_Droste wrote:
> zaks.anna wrote:
> > This should probably go under the 'option' package. What do you think?
> Do you mean the 'optin' package? I could not find an 'option' package in `Checkers.td`.
Yes, 'option'. I think it got spell checked.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43
@@ +42,3 @@
+private:
+  const std::unique_ptr<MPICheckerPathSensitive> CheckerSens;
+
----------------
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,

================
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);
+    }
----------------
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.)

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:21
@@ +20,3 @@
+
+namespace util {
+
----------------
Alexander_Droste wrote:
> zaks.anna wrote:
> > I'd like to remove the Utility.cpp completely by either making the helper functions static or moving them to other shared components.
> So where shall we put `sourceRange()`? It is only used by the BugReporter class.
> I could make it a member function of the Reporter class. Or would you prefer this
> as a member function of `MemRegion`?
I don't see why we should't add it to MemRegion. Let's submit that as a separate patch. (If someone else has a better idea they'll tell us:))


http://reviews.llvm.org/D12761





More information about the cfe-commits mailing list