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

Dmitri Gribenko via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 11 15:42:06 PDT 2015 

gribozavr added inline comments.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+  HelpText<"Checks MPI code written in C">,
+  DescFile<"MPIChecker.cpp">;
----------------
Does it only works with C code, or does it work with C++ code that uses C bindings for MPI?  I think it is safe to describe it as "Checks MPI code".

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:1
@@ +1,2 @@
+//===-- Container.hpp - convenience templates for containers ----*- C++ -*-===//
+//
----------------
The file should use the `.h` extension (here and everywhere else in the patch).

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:10-11
@@ +9,4 @@
+///
+/// \file
+/// This file defines convenience templates for C++ container class usage.
+///
----------------
This file re-invents a lot of APIs that are currently under review by the C++ committee under the Ranges effort.  I think most of the wrappers are more or less trivial and should use STL directly, or the wrappers should match exactly the currently proposed STL APIs.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:15
@@ +14,3 @@
+
+#ifndef CONTAINER_HPP_XM1FDRVJ
+#define CONTAINER_HPP_XM1FDRVJ
----------------
Please look at other files in Clang and follow the same include guard style (here and everywhere else in the patch).

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:22
@@ +21,3 @@
+
+/// \brief Check if given element is contained.
+///
----------------
Please drop `\brief` everywhere.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:24-25
@@ +23,4 @@
+///
+/// \param container
+/// \param elementToCheck
+///
----------------
If there is no description for the parameter, please drop `\param`.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:27
@@ +26,3 @@
+///
+/// \returns true if element contained
+template <typename T, typename E>
----------------
This is the gist of the documentation.  I think the doc comment should be just one line, `/// Returns true if \p container contains \p element.`

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:29
@@ +28,3 @@
+template <typename T, typename E>
+bool isContained(const T &container, const E &elementToCheck) {
+  return std::find(container.begin(), container.end(), elementToCheck) !=
----------------
I'd prefer the parameter to be called just `Element`.  The function name should probably be just `contains`.

Also, please follow the LLVM naming style everywhere (variable names start with a capital letter).

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:39
@@ +38,3 @@
+///
+/// \returns true if element that matched predicate is contained
+template <typename T, typename P>
----------------
`/// Returns true if \p container contains an element for which \p predicate returns true.`


================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:46-50
@@ +45,7 @@
+
+/// \brief Deletes first appearance of given element.
+///
+/// \param container
+/// \param elementToErase
+template <typename T, typename E> void erase(T &container, E &elementToErase) {
+  auto it = std::find(container.begin(), container.end(), elementToErase);
----------------
Same comments as above apply to this function as well as the rest of the patch.  I wouldn't be duplicating them over and over again, please scan the whole patch.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:67
@@ +66,3 @@
+
+/// \brief Deletes first appearance of given pointer.
+///
----------------
Why is this a separate overload?

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:88-109
@@ +87,24 @@
+
+/// \brief Deletes element at given index.
+///
+/// \param container
+/// \param index
+template <typename T> void eraseIndex(T &container, size_t idx) {
+  container.erase(container.begin() + idx);
+}
+
+/// \brief Sort with default criterion.
+///
+/// \param container
+template <typename T> void sort(T &container) {
+  std::sort(container.begin(), container.end());
+}
+
+/// \brief Sort by given predicate.
+///
+/// \param container
+/// \param predicate
+template <typename T, typename P> void sortPred(T &container, P predicate) {
+  std::sort(container.begin(), container.end(), predicate);
+}
+
----------------
I question the value of such trivial wrappers.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.hpp:22
@@ +21,3 @@
+
+namespace mpi {
+
----------------
The namespace should be nested under `clang`, we shouldn't be claiming the top-level `mpi` namespace.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.hpp:45-48
@@ +44,6 @@
+  ///
+  /// \param call expression to report mismatch for
+  /// \param argument indices
+  /// \param buffer type
+  /// \param MPI type
+  void reportTypeMismatch(const clang::CallExpr *const,
----------------
The syntax of `\param` is: `\param <parameter name> <description>`, these lines are missing the parameter names.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.hpp:49-51
@@ +48,5 @@
+  /// \param MPI type
+  void reportTypeMismatch(const clang::CallExpr *const,
+                          const std::pair<size_t, size_t> &, clang::QualType,
+                          std::string) const;
+
----------------
The forward declarations should include parameter names for documentation and readability purposes.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.hpp:104
@@ +103,3 @@
+
+  const clang::Decl *currentFunctionDecl_{nullptr};
+
----------------
Naming class members with a trailing underscore is not a part of LLVM style (here and everywhere else in the patch).  Please follow the LLVM coding guidelines: http://llvm.org/docs/CodingStandards.html

Also, I think using `= nullptr` is more readable in this case.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.cpp:22
@@ +21,3 @@
+
+void MPICheckerAST::initMPITypeContainer() {
+  mpiTypes_ = {"MPI_BYTE",
----------------
MPICH has its header files annotated for Clang for a few years already, I think we should be using the annotations instead of textually matching the constants.  Moreover, some users are using those annotations for their own datatypes.

================
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerAST.hpp:101
@@ +100,3 @@
+  MPIFunctionClassifier funcClassifier_;
+  llvm::SmallVector<std::string, 32> mpiTypes_;
+  MPIBugReporter bugReporter_;
----------------
This should be a hashtable-based set, like `llvm::DenseSet`.  Scanning this vector over and over again is wasteful.

================
Comment at: tools/clang/test/Analysis/MPIChecker.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -I/usr/include/ -I/usr/local/include/ -analyze -analyzer-checker=mpi.MPI-Checker -verify %s
+
----------------
Please drop `-I/usr/...` flags here.

================
Comment at: tools/clang/test/Analysis/MPIChecker.c:3
@@ +2,3 @@
+
+#include <mpi.h>
+#include <complex.h>
----------------
We can not include `mpi.h` directly here, because we can not require all developers to have MPI installed.  We need to use a mock header that is checked into Clang's repository.

Even more, the implementation details of `mpi.h` are in fact implementation details.  We need to make sure that the checks work with MPICH-style and OpenMPI-style definitions.  Please take a look at the incomplete `mpi.h` mock that I have in `test/Sema/warn-type-safety-mpi-hdf5.c`.


http://reviews.llvm.org/D12761

More information about the cfe-commits mailing list