Alexander_Droste added a comment. Hi Anna,
thanks for having a look once more! ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75 @@ -74,1 +74,3 @@ +def MPI : Package<"mpi">; + ---------------- 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`. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:24 @@ +23,3 @@ +namespace clang { +namespace mpi { + ---------------- zaks.anna wrote: > Should this be clang::ento::mpi? Alternatively, you could place everything > into a single file and have it live in anonymous namespace. This would also > be more consistent with the existing checkers. I would prefer to put this into `clang::ento::mpi`, rather than having everything in a single file. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h:40 @@ +39,3 @@ + + // ast reports –––––––––––––––––––––––––––––––––––––––––––––––––––––––––– + ---------------- zaks.anna wrote: > Looks like some of the AST stuff was deleted and some was kept. Please, > either remove all of it or keep all of it in. Sorry about the inconsistent change. I will remove the AST related functionality completety from this patch, as it will be part of the clang-tidy patch. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43 @@ +42,3 @@ +private: + const std::unique_ptr<MPICheckerPathSensitive> CheckerSens; + ---------------- 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? ================ 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: > 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. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:79 @@ +78,3 @@ + if (!ReqRegions.empty()) { + Ctx.addTransition(State); + } ---------------- zaks.anna wrote: > Don't forget to specify a predecessor here once the code above changes. Will do. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h:87 @@ +86,3 @@ + const MPIFunctionClassifier FuncClassifier; + MPIBugReporter BugReporter; +}; ---------------- zaks.anna wrote: > Please, use a name other than 'BugReporter' to avoid confusing it with the > BugReporter in the analyzer. I see, that could be confusing. Maybe renaming the variable to BReporter will do. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h:45 @@ +44,3 @@ + bool isAllgatherType(const clang::IdentifierInfo *const IdentInfo) const; + bool isAlltoallType(const clang::IdentifierInfo *const IdentInfo) const; + bool isReduceType(const clang::IdentifierInfo *const IdentInfo) const; ---------------- zaks.anna wrote: > Some of these classifier functions are not used either.. These model distinct MPI function classes. I agree that it would be better to remove the unused ones, in order to keep the interface as narrow as possible. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:56 @@ +55,3 @@ +struct RequestMap {}; +typedef llvm::ImmutableMap<const clang::ento::MemRegion *, + clang::ento::mpi::Request> RequestMapImpl; ---------------- zaks.anna wrote: > Let's add some documentation on why you are not using the standard macro > REGISTER_MAP_WITH_PROGRAMSTATE. (Might help to avoid confusion lear on.) Sure, I can do that. ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:21 @@ +20,3 @@ + +namespace util { + ---------------- 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`? ================ Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:47 @@ +46,3 @@ + +const clang::IdentifierInfo *getIdentInfo(const clang::CallExpr *CE) { + if (CE->getDirectCallee()) { ---------------- zaks.anna wrote: > This is not used.. These are also leftovers from the AST checks. ================ Comment at: tools/clang/test/Analysis/MPIChecker.cpp:112 @@ +111,3 @@ + MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, + &sendReq1); // Request is previously used by nonblocking call here. +} // expected-warning{{Request 'sendReq1' has no matching wait.}} ---------------- zaks.anna wrote: > This is not testing the extra information provided by bug reporter visitor; > you should use "// expected-note {...}" I didn't know about `expected-note`, thanks! http://reviews.llvm.org/D12761 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits