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@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
