[PATCH] D21962: MPITypeMismatchCheck for Clang-Tidy

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thanks for addressing the comments. The code looks better now. My main concern at this point is that the name and the location of the check: "misc" is a place for the stuff we can't find a better place for, but here I guess we clearly want a separate module and a subdirectory for MPI-related checks (MPITidyModule, "mpi/"). The check name should be updated accordingly: "mpi-type-mismatch".

A few more comments inline.


================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:212
@@ +211,3 @@
+
+  static std::map<std::string, std::string> FixedWidthMatches = {
+      {"int8_t", "MPI_INT8_T"},     {"int16_t", "MPI_INT16_T"},
----------------
`llvm::StringMap` should be more efficient here.

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:218
@@ +217,3 @@
+
+  StringRef TypedefToCompare = Typedef->getDecl()->getQualifiedNameAsString();
+  // Check if the typedef is known and not matching the MPI datatype.
----------------
`getQualifiedNameAsString` returns a `std::string`, which will be destroyed at the end of this statement. `TypedefToCompare` will be a dangling reference then. Please change `StringRef` to `std::string` here.

Another question is whether you need `getQualifiedNameAsString`, which is rather expensive. Maybe you could use `getName` (it returns a `StringRef` and is relatively cheap) and additionally check that the name is in the global namespace?

================
Comment at: clang-tidy/misc/MpiTypeMismatchCheck.cpp:220
@@ +219,3 @@
+  // Check if the typedef is known and not matching the MPI datatype.
+  if (FixedWidthMatches.find(TypedefToCompare) != FixedWidthMatches.end() &&
+      FixedWidthMatches.at(TypedefToCompare) != MPIDatatype) {
----------------
Repeated lookups into a map are wasteful. Please store the result of `find` and use it to get the value.


https://reviews.llvm.org/D21962