[PATCH] D21962: MPITypeMismatchCheck for Clang-Tidy

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:147
@@ +146,3 @@
+const Type *MpiTypeMismatchCheck::argumentType(const CallExpr *const CE,
+                                               const size_t idx) {
+  const QualType QT = CE->getArg(idx)->IgnoreImpCasts()->getType();
----------------
What about this comment?

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:152
@@ +151,3 @@
+  } else if (QT.getTypePtr()->isPointerType()) {
+    return QT.getTypePtr()->getPointeeType()->getBaseElementTypeUnsafe();
+  }
----------------
What about this comment? I find it convenient to mark comments "Done" when I address them, so it's easier to track the action items.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:259
@@ +258,3 @@
+  auto Loc = ArgumentExpression->getSourceRange().getBegin();
+  diag(Loc, (llvm::Twine("Buffer type '") + BufferTypeName +
+             "' does not match the MPI datatype '" + MPIDatatype + "'.")
----------------
Diagnostics are not complete sentences, so they should not start with a capital letter and should not end with a period.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:259
@@ +258,3 @@
+  auto Loc = ArgumentExpression->getSourceRange().getBegin();
+  diag(Loc, (llvm::Twine("Buffer type '") + BufferTypeName +
+             "' does not match the MPI datatype '" + MPIDatatype + "'.")
----------------
alexfh wrote:
> Diagnostics are not complete sentences, so they should not start with a capital letter and should not end with a period.
Please use formatting placeholders instead of concatenation: `diag("buffer type '%0' does not match ...") << BufferTypeName << ....;`. 

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:271
@@ +270,3 @@
+    bool Error = false;
+    std::string BufferTypeName;
+
----------------
It looks like you're trying to avoid unnecessary allocations by passing this buffer into the calls below. It would probably be even more efficient, if you moved this variable out of the loop.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.h:128
@@ +127,3 @@
+
+  static llvm::SmallVector<std::string, 32> MPITypes;
+  static std::multimap<BuiltinType::Kind, std::string> BuiltinMatches;
----------------
These static members and corresponding methods don't have to be in the class. My comment might have been ambiguous, but I was actually suggesting to change the methods that use these maps to free-standing static functions in the .cpp file and make these maps static local variables in the corresponding functions.


http://reviews.llvm.org/D21962