[PATCH] D21962: MPITypeMismatchCheck for Clang-Tidy

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Please re-generate the diff with full-context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

A bunch of style comments.


================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:86
@@ +85,3 @@
+
+void MpiTypeMismatchCheck::initAlltypesSet() {
+  AllTypes = {"MPI_C_BOOL",
----------------
I don't think this needs to be a method of the check class. I'd say a `static bool isMPIDataType(StringRef Type)` is a proper interface, and it can have the list of types as a static local variable.



================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:122
@@ +121,3 @@
+
+bool MpiTypeMismatchCheck::isMPITypeMatching(
+    const std::multimap<BuiltinType::Kind, std::string> &MultiMap,
----------------
Since this method doesn't need any members of the class, it should be a static function instead.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:130
@@ +129,3 @@
+    if (ItPair.first->second == MPIDatatype) {
+      Matches = true;
+      break;
----------------
Just `return true;`

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:138
@@ +137,3 @@
+
+StringRef MpiTypeMismatchCheck::argumentAsStringRef(
+    const CallExpr *const CE, const size_t idx,
----------------
Looks like you can use getText from clang/include/clang/Tooling/FixIt.h.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:146
@@ +145,3 @@
+
+const Type *MpiTypeMismatchCheck::argumentType(const CallExpr *const CE,
+                                               const size_t idx) const {
----------------
Since this method doesn't need any members of the class, it should be a static function instead.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:151
@@ +150,3 @@
+    return QT.getTypePtr()->getArrayElementTypeNoTypeQual();
+  } else if (QT.getTypePtr()->isPointerType()) {
+    return QT.getTypePtr()->getPointeeType()->getBaseElementTypeUnsafe();
----------------
No `else` after `return`, please. http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:222
@@ +221,3 @@
+
+bool MpiTypeMismatchCheck::isCXXComplexTypeMatching(
+    const TemplateSpecializationType *const Template,
----------------
Should probably be a static function as well. `ComplexCXXMatches` can be a static local variable instead.

Same applies for the other similar methods.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:233
@@ +232,3 @@
+    BufferTypeName =
+        (llvm::Twine("complex<") + Builtin->getName(LangOptions()) + ">").str();
+    return isMPITypeMatching(ComplexCXXMatches, Builtin->getKind(),
----------------
Please use actual language options (e.g. from MatchResult).

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:262
@@ +261,3 @@
+void MpiTypeMismatchCheck::checkArguments(
+    const SmallVector<const Type *, 1> &BufferTypes,
+    const SmallVector<StringRef, 1> &MPIDatatypes,
----------------
Use ArrayRef<const Type *> instead.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:268
@@ +267,3 @@
+    const Type *BT = BufferTypes[i];
+    bool Error{false};
+    std::string BufferTypeName;
----------------
Please use assignment-style initialization. http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:271
@@ +270,3 @@
+
+    // typedef
+    if (const TypedefType *Typedef = BT->getAs<TypedefType>()) {
----------------
Please use proper capitalization and punctuation in comments. 

http://llvm.org/docs/CodingStandards.html#commenting

"When writing comments, write them as English prose, which means they should use proper capitalization, punctuation, etc."

Actually, I'm not sure these comments are helpful.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:272
@@ +271,3 @@
+    // typedef
+    if (const TypedefType *Typedef = BT->getAs<TypedefType>()) {
+      Error = !isTypedefTypeMatching(Typedef, BufferTypeName, MPIDatatypes[i]);
----------------
Use `const auto *` when the type of the variable is obvious (e.g. when the initializer literally spells it, like here).

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:276
@@ +275,3 @@
+    // complex c type
+    else if (const ComplexType *Complex = BT->getAs<ComplexType>()) {
+      Error = !isCComplexTypeMatching(Complex, BufferTypeName, MPIDatatypes[i]);
----------------
`else` should be on the same line as the preceding `}`.

================
Comment at: test/clang-tidy/misc-mpi-type-mismatch.cpp:4
@@ +3,3 @@
+// #include "clang/../../test/Analysis/MPIMock.h"
+#include "/Users/lx/Documents/Text/Code/Open_Source/llvm_trunk_git/repo/tools/clang/test/Analysis/MPIMock.h"
+
----------------
We usually put headers needed for tests to a subdiretory (named after the test name) of the `Inputs` directory. See test/clang-tidy/modernize-loop-convert-extra.cpp, for example.


http://reviews.llvm.org/D21962