[PATCH] [clang-tidy] Add a checker for swapped literal arguments.

Daniel Jasper djasper at google.com
Thu Jul 10 03:20:17 PDT 2014


================
Comment at: clang-tidy/misc/CMakeLists.txt:6
@@ -5,2 +5,3 @@
   BoolPointerImplicitConversion.cpp
+  ImplicitArgumentConversion.cpp
   MiscTidyModule.cpp
----------------
ImplicitArgumentConversion is not exactly what this does, I think. Maybe SwappedArgumentsCheck?

================
Comment at: clang-tidy/misc/ImplicitArgumentConversion.cpp:33
@@ +32,3 @@
+  for (auto *Parm : Call->arguments()) {
+    // We only care about literals.
+    const Expr *PP = Parm->IgnoreImpCasts();
----------------
Why do we only care about literals?

================
Comment at: clang-tidy/misc/ImplicitArgumentConversion.cpp:34
@@ +33,3 @@
+    // We only care about literals.
+    const Expr *PP = Parm->IgnoreImpCasts();
+    if (!isa<CharacterLiteral>(PP) && !isa<CXXBoolLiteralExpr>(PP) &&
----------------
Maybe IgnoreParenImpCasts()?

================
Comment at: clang-tidy/misc/ImplicitArgumentConversion.cpp:52
@@ +51,3 @@
+  // most likely a swap.
+  if (NumConversions >= 2 && DeclTypes == CallTypes) {
+    diag(Call->getLocStart(),
----------------
Mhmm. This seems like a very crude way of doing this. Might be ok for a first iteration, but it seems like we could actually be much smarter and check if there is a re-ordering that would fit better (and even extend the analysis to more than two parameters, no)?

================
Comment at: clang-tidy/misc/ImplicitArgumentConversion.h:18
@@ +17,3 @@
+
+/// \brief 
+class ImplicitArgumentConversion : public ClangTidyCheck {
----------------
This is VERY brief ;-)

http://reviews.llvm.org/D4457






More information about the cfe-commits mailing list