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