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