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