[clang-tidy] Add a checker that removes deducible arguments from std::make_pair

[bkramer]

Summary

Those may be incompatible with C++11 and are unnecessary. We suggest
removing the template arguments when they match the types of the make_pair
arguments or replace it with std::pair and explicit template arguments when
not.

[alexfh]

Looks good provided you address the comments.

clang-tidy/google/ExplicitMakePairCheck.cpp

41 ↗

(On Diff #11386)

nit: I'd better not crash here even if someone defines their own ::std::make_pair.

43 ↗

(On Diff #11386)

nit: Missing comma after "match"?

test/clang-tidy/google-explicit-make-pair.cpp

16 ↗

(On Diff #11386)

Please add tests with make_pair in template instantiations and in a macro.

[bkramer]

Closed by commit rL213058 (authored by d0k).

[alexfh]

clang-tools-extra/trunk/test/clang-tidy/google-explicit-make-pair.cpp

18

I assume the check should suggest replacements in cases where there are no dependent types, but could you add a test? And one more instantiation, so that we actually test a case where something can go wrong due to templates.

[bkramer]

clang-tools-extra/trunk/test/clang-tidy/google-explicit-make-pair.cpp

18

We completely ignore code in template instantiations at the moment. It would be nice to check for code in templates that does not contain any dependencies on the template arguments but it's not tricky to match (or do we have an ASTMatcher for it?)

[alexfh]

clang-tools-extra/trunk/test/clang-tidy/google-explicit-make-pair.cpp

18

Yes, we ignore code in template _instantiations_. However, template _definitions_ are handled as usual. See a test for this in test/clang-tidy/avoid-c-style-casts.cpp:78

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
+  ExplicitMakePairCheck.cpp
   GoogleTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/google/ExplicitMakePairCheck.h

+//===--- ExplicitMakePairCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_EXPLICIT_MAKE_PAIR_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_EXPLICIT_MAKE_PAIR_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace build {
+
+/// \brief Check that make_pair's template arguments are deduced.
+///
+/// G++ 4.6 in C++11 mode fails badly if make_pair's template arguments are
+/// specified explicitly, and such use isn't intended in any case.
+///
+/// Corresponding cpplint.py check name: 'build/explicit_make_pair'.
+class ExplicitMakePairCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace build
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_EXPLICIT_MAKE_PAIR_CHECK_H

clang-tools-extra/trunk/clang-tidy/google/ExplicitMakePairCheck.cpp

+//===--- ExplicitMakePairCheck.cpp - clang-tidy -----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExplicitMakePairCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+
+namespace ast_matchers {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace ast_matchers
+
+namespace tidy {
+namespace build {
+
+void
+ExplicitMakePairCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  // Look for std::make_pair with explicit template args. Ignore calls in
+  // templates.
+  Finder->addMatcher(
+      callExpr(unless(hasAncestor(decl(anyOf(
+                   recordDecl(ast_matchers::isTemplateInstantiation()),
+                   functionDecl(ast_matchers::isTemplateInstantiation()))))),
+               has(declRefExpr(hasExplicitTemplateArgs()).bind("declref")),
+               callee(functionDecl(hasName("::std::make_pair")))).bind("call"),
+      this);
+}
+
+void ExplicitMakePairCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+  const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref");
+
+  // Sanity check: The use might have overriden ::std::make_pair.
+  if (Call->getNumArgs() != 2)
+    return;
+
+  const Expr *Arg0 = Call->getArg(0)->IgnoreParenImpCasts();
+  const Expr *Arg1 = Call->getArg(1)->IgnoreParenImpCasts();
+
+  // If types don't match, we suggest replacing with std::pair and explicit
+  // template arguments. Otherwise just remove the template arguments from
+  // make_pair.
+  if (Arg0->getType() != Call->getArg(0)->getType() ||
+      Arg1->getType() != Call->getArg(1)->getType()) {
+    diag(Call->getLocStart(), "for C++11-compatibility, use pair directly")
+        << FixItHint::CreateReplacement(
+            SourceRange(DeclRef->getLocStart(), DeclRef->getLAngleLoc()),
+            "std::pair<");
+  } else {
+    diag(Call->getLocStart(),
+         "for C++11-compatibility, omit template arguments from make_pair")
+        << FixItHint::CreateRemoval(
+            SourceRange(DeclRef->getLAngleLoc(), DeclRef->getRAngleLoc()));
+  }
+}
+
+} // namespace build
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.cpp - clang-tidy --------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
+#include "ExplicitMakePairCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 class GoogleModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
+        "google-build-explicit-make-pair",
+        new ClangTidyCheckFactory<build::ExplicitMakePairCheck>());
+    CheckFactories.addCheckFactory(
         "google-explicit-constructor",
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
     CheckFactories.addCheckFactory(
         "google-readability-casting",
         new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());
   }
 };
 

clang-tools-extra/trunk/test/clang-tidy/google-explicit-make-pair.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-build-explicit-make-pair %t
+// REQUIRES: shell
+
+namespace std {
+template <class T1, class T2>
+struct pair {
+  pair(T1 x, T2 y) {}
+};
+
+template <class T1, class T2>
+pair<T1, T2> make_pair(T1 x, T2 y) {
+  return pair<T1, T2>(x, y);
+}
+}
+
+template <typename T>
+void templ(T a, T b) {
+  std::make_pair<T, unsigned>(a, b);
+}
+
+void test(int i) {
+  std::make_pair<int, int>(i, i);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: for C++11-compatibility, omit template arguments from make_pair
+// CHECK-FIXES: std::make_pair(i, i)
+
+  std::make_pair<unsigned, int>(i, i);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: for C++11-compatibility, use pair directly
+// CHECK-FIXES: std::pair<unsigned, int>(i, i)
+
+  std::make_pair<int, unsigned>(i, i);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: for C++11-compatibility, use pair directly
+// CHECK-FIXES: std::pair<int, unsigned>(i, i)
+
+#define M std::make_pair<int, unsigned>(i, i);
+M
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: for C++11-compatibility, use pair directly
+// Can't fix in macros.
+// CHECK-FIXES: #define M std::make_pair<int, unsigned>(i, i);
+// CHECK-FIXES-NEXT: M
+
+  templ(i, i);
+
+  std::make_pair(i, 1); // no-warning
+}