zaks.anna added a comment.
Alexander,
This patch is in a pretty good shape. I am fine with committing it and
iterating with smaller updates in tree if it is more convenient for you.
One task that I would like to very strongly encourage is running this on a lot
of code. You will find a lot of issues this way that are hard to discover
during code review. Both false positives and crashes.
Thanks!
Anna.
================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:96
@@ +95,3 @@
+ if (const CallExpr *CE = clang::dyn_cast<CallExpr>(SP->getStmt())) {
+
+ auto FuncIdentifier = CE->getDirectCallee()->getIdentifier();
----------------
The advantage of using the state is that it will be much more robust to any
further changes to the compiler/checker because you will not be pattern
matching the AST but instead will be checking the state, which the core
reasoning is based on. One example that comes to mind is indirect calls. You
will reduce the amount of code here as well, simplifying maintainability. This
is the pattern we use in other checkers as well, so there is a remote chance we
could introduce a new simplified API that will do the walk for the checker
writers.
With respect to your example. Does it come up in practice? Wouldn't you warn on
the second nonblocking request anyway? Could you add such an example to the
tests? (Would be good in any case. If you leave the code as is, you can point
to that example as the motivation.)
================
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:87
@@ +86,3 @@
+ }
+ // A wait has no matching nonblocking call.
+ BReporter->reportUnmatchedWait(PreCallEvent, ReqRegion, ErrorNode);
----------------
This is done, right?
================
Comment at: test/Analysis/MPIChecker.cpp:99
@@ +98,3 @@
+ MPI_Wait(&req, MPI_STATUS_IGNORE);
+}
+
----------------
This are explaining the path on which the problem occurs; the users will see
them as well. There should not be a lot of those, you do not have a lot of
conditions. Would it be reasonable to change the tests to incorporate those.
Other alternative is to have another tests file that tests the notes in that
mode.
What do you think?
================
Comment at: test/Analysis/MPIChecker.cpp:114
@@ +113,3 @@
+
+void doubleNonblocking4() {
+ int rank = 0;
----------------
> I would then simply create a new pair of .cpp and .h files in the test folder
> where I define those functions so that the MPI-Checker tests can use them.
You do not have to do that. You could just declare the functions and not define
them. It will be equivalent to having the definitions in the other TUs.
http://reviews.llvm.org/D12761
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
