[clang-tidy] Create clang-tidy module modernize. Add pass-by-value check.

This is the first step for migrating cppmodernize to clang-tidy.

http://reviews.llvm.org/D11946

Patch by Angel Garcia!

llvm-svn: 245045

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

@@ -31,5 +31,6 @@ add_subdirectory(tool)
 add_subdirectory(llvm)
 add_subdirectory(google)
 add_subdirectory(misc)
+add_subdirectory(modernize)
 add_subdirectory(readability)
 add_subdirectory(utils)

clang-tools-extra/clang-tidy/Makefile

@@ -11,6 +11,6 @@ CLANG_LEVEL := ../../..
 LIBRARYNAME := clangTidy
 include $(CLANG_LEVEL)/../../Makefile.config
 
-DIRS = utils readability llvm google misc tool
+DIRS = utils readability llvm google misc modernize tool
 
 include $(CLANG_LEVEL)/Makefile

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

@@ -0,0 +1,14 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyModernizeModule
+  ModernizeTidyModule.cpp
+  PassByValueCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyReadabilityModule
+  )

clang-tools-extra/clang-tidy/modernize/Makefile

@@ -0,0 +1,12 @@
+##===- clang-tidy/modernize/Makefile -----------------------*- Makefile -*-===##
+#
+#                     The LLVM Compiler Infrastructure
+#
+# This file is distributed under the University of Illinois Open Source
+# License. See LICENSE.TXT for details.
+#
+##===----------------------------------------------------------------------===##
+CLANG_LEVEL := ../../../..
+LIBRARYNAME := clangTidyModernizeModule
+
+include $(CLANG_LEVEL)/Makefile

clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

@@ -0,0 +1,46 @@
+//===--- ModernizeTidyModule.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 "PassByValueCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+class ModernizeModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    auto &Opts = Options.CheckOptions;
+    Opts["modernize-pass-by-value.IncludeStyle"] = "llvm"; // Also: "google".
+    return Options;
+  }
+};
+
+// Register the ModernizeTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<ModernizeModule> X("modernize-module",
+                                                       "Add modernize checks.");
+
+} // namespace modernize
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ModernizeModule.
+volatile int ModernizeModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

@@ -0,0 +1,223 @@
+//===--- PassByValueCheck.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 "PassByValueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+using namespace llvm;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// \brief Matches move-constructible classes.
+///
+/// Given
+/// \code
+///   // POD types are trivially move constructible.
+///   struct Foo { int a; };
+///
+///   struct Bar {
+///     Bar(Bar &&) = deleted;
+///     int a;
+///   };
+/// \endcode
+/// recordDecl(isMoveConstructible())
+///   matches "Foo".
+AST_MATCHER(CXXRecordDecl, isMoveConstructible) {
+  for (const CXXConstructorDecl *Ctor : Node.ctors()) {
+    if (Ctor->isMoveConstructor() && !Ctor->isDeleted())
+      return true;
+  }
+  return false;
+}
+
+/// \brief Matches non-deleted copy constructors.
+///
+/// Given
+/// \code
+///   struct Foo { Foo(const Foo &) = default; };
+///   struct Bar { Bar(const Bar &) = deleted; };
+/// \endcode
+/// constructorDecl(isNonDeletedCopyConstructor())
+///   matches "Foo(const Foo &)".
+AST_MATCHER(CXXConstructorDecl, isNonDeletedCopyConstructor) {
+  return Node.isCopyConstructor() && !Node.isDeleted();
+}
+
+static TypeMatcher constRefType() {
+  return lValueReferenceType(pointee(isConstQualified()));
+}
+
+static TypeMatcher nonConstValueType() {
+  return qualType(unless(anyOf(referenceType(), isConstQualified())));
+}
+
+/// \brief Whether or not \p ParamDecl is used exactly one time in \p Ctor.
+///
+/// Checks both in the init-list and the body of the constructor.
+static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor,
+                                     const ParmVarDecl *ParamDecl) {
+  /// \brief \c clang::RecursiveASTVisitor that checks that the given
+  /// \c ParmVarDecl is used exactly one time.
+  ///
+  /// \see ExactlyOneUsageVisitor::hasExactlyOneUsageIn()
+  class ExactlyOneUsageVisitor
+      : public RecursiveASTVisitor<ExactlyOneUsageVisitor> {
+    friend class RecursiveASTVisitor<ExactlyOneUsageVisitor>;
+
+  public:
+    ExactlyOneUsageVisitor(const ParmVarDecl *ParamDecl)
+        : ParamDecl(ParamDecl) {}
+
+    /// \brief Whether or not the parameter variable is referred only once in
+    /// the
+    /// given constructor.
+    bool hasExactlyOneUsageIn(const CXXConstructorDecl *Ctor) {
+      Count = 0;
+      TraverseDecl(const_cast<CXXConstructorDecl *>(Ctor));
+      return Count == 1;
+    }
+
+  private:
+    /// \brief Counts the number of references to a variable.
+    ///
+    /// Stops the AST traversal if more than one usage is found.
+    bool VisitDeclRefExpr(DeclRefExpr *D) {
+      if (const ParmVarDecl *To = dyn_cast<ParmVarDecl>(D->getDecl())) {
+        if (To == ParamDecl) {
+          ++Count;
+          if (Count > 1) {
+            // No need to look further, used more than once.
+            return false;
+          }
+        }
+      }
+      return true;
+    }
+
+    const ParmVarDecl *ParamDecl;
+    unsigned Count;
+  };
+
+  return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor);
+}
+
+/// \brief Find all references to \p ParamDecl across all of the
+/// redeclarations of \p Ctor.
+static SmallVector<const ParmVarDecl *, 2>
+collectParamDecls(const CXXConstructorDecl *Ctor,
+                  const ParmVarDecl *ParamDecl) {
+  SmallVector<const ParmVarDecl *, 2> Results;
+  unsigned ParamIdx = ParamDecl->getFunctionScopeIndex();
+
+  for (const FunctionDecl *Redecl : Ctor->redecls())
+    Results.push_back(Redecl->getParamDecl(ParamIdx));
+  return Results;
+}
+
+PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm" ?
+                   IncludeSorter::IS_LLVM : IncludeSorter::IS_Google) {}
+
+void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle",
+                IncludeStyle == IncludeSorter::IS_LLVM ? "llvm" : "google");
+}
+
+void PassByValueCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      constructorDecl(
+          forEachConstructorInitializer(
+              ctorInitializer(
+                  // Clang builds a CXXConstructExpr only whin it knows which
+                  // constructor will be called. In dependent contexts a
+                  // ParenListExpr is generated instead of a CXXConstructExpr,
+                  // filtering out templates automatically for us.
+                  withInitializer(constructExpr(
+                      has(declRefExpr(to(
+                          parmVarDecl(
+                              hasType(qualType(
+                                  // Match only const-ref or a non-const value
+                                  // parameters. Rvalues and const-values
+                                  // shouldn't be modified.
+                                  anyOf(constRefType(), nonConstValueType()))))
+                              .bind("Param")))),
+                      hasDeclaration(constructorDecl(
+                          isNonDeletedCopyConstructor(),
+                          hasDeclContext(recordDecl(isMoveConstructible())))))))
+                  .bind("Initializer")))
+          .bind("Ctor"),
+      this);
+}
+
+void PassByValueCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Inserter.reset(new IncludeInserter(Compiler.getSourceManager(),
+                                     Compiler.getLangOpts(), IncludeStyle));
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("Ctor");
+  const auto *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("Param");
+  const auto *Initializer =
+      Result.Nodes.getNodeAs<CXXCtorInitializer>("Initializer");
+  SourceManager &SM = *Result.SourceManager;
+
+  // If the parameter is used or anything other than the copy, do not apply
+  // the changes.
+  if (!paramReferredExactlyOnce(Ctor, ParamDecl))
+    return;
+
+  auto Diag = diag(ParamDecl->getLocStart(), "pass by value and use std::move");
+
+  // Iterate over all declarations of the constructor.
+  for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+    auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+    auto RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+
+    // Do not replace if it is already a value, skip.
+    if (RefTL.isNull())
+      continue;
+
+    TypeLoc ValueTL = RefTL.getPointeeLoc();
+    auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getLocStart(),
+                                                    ParamTL.getLocEnd());
+    std::string ValueStr =
+        Lexer::getSourceText(
+            CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+            Result.Context->getLangOpts())
+            .str();
+    ValueStr += ' ';
+    Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+  }
+
+  // Use std::move in the initialization list.
+  Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")")
+       << FixItHint::CreateInsertion(
+              Initializer->getLParenLoc().getLocWithOffset(1), "std::move(");
+
+  auto Insertion =
+      Inserter->CreateIncludeInsertion(SM.getMainFileID(), "utility",
+                                       /*IsAngled=*/true);
+  if (Insertion.hasValue())
+    Diag << Insertion.getValue();
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h

@@ -0,0 +1,39 @@
+//===--- PassByValueCheck.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_MODERNIZE_PASS_BY_VALUE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H
+
+#include "../ClangTidy.h"
+#include "../IncludeInserter.h"
+
+#include <memory>
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+class PassByValueCheck : public ClangTidyCheck {
+public:
+  PassByValueCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerPPCallbacks(clang::CompilerInstance &Compiler) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::unique_ptr<IncludeInserter> Inserter;
+  const IncludeSorter::IncludeStyle IncludeStyle;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H

clang-tools-extra/clang-tidy/tool/CMakeLists.txt

@@ -13,6 +13,7 @@ target_link_libraries(clang-tidy
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
+  clangTidyModernizeModule
   clangTidyReadabilityModule
   clangTooling
   )

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

@@ -346,6 +346,10 @@ static int LLVM_ATTRIBUTE_UNUSED GoogleModuleAnchorDestination = GoogleModuleAnc
 extern volatile int MiscModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination = MiscModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ModernizeModule.
+extern volatile int ModernizeModuleAnchorSource;
+static int ModernizeModuleAnchorDestination = ModernizeModuleAnchorSource;
+
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination = ReadabilityModuleAnchorSource;

clang-tools-extra/clang-tidy/tool/Makefile

@@ -17,9 +17,9 @@ TOOL_NO_EXPORTS = 1
 include $(CLANG_LEVEL)/../../Makefile.config
 LINK_COMPONENTS := $(TARGETS_TO_BUILD) asmparser bitreader support mc option
 USEDLIBS = clangTidy.a clangTidyLLVMModule.a clangTidyGoogleModule.a \
-	   clangTidyMiscModule.a clangTidyReadability.a clangTidyUtils.a \
-	   clangStaticAnalyzerFrontend.a clangStaticAnalyzerCheckers.a \
-	   clangStaticAnalyzerCore.a \
+	   clangTidyMiscModule.a clangTidyModernizeModule.a clangTidyReadability.a \
+	   clangTidyUtils.a clangStaticAnalyzerFrontend.a \
+	   clangStaticAnalyzerCheckers.a clangStaticAnalyzerCore.a \
 	   clangFormat.a clangASTMatchers.a clangTooling.a clangToolingCore.a \
 	   clangFrontend.a clangSerialization.a clangDriver.a clangParse.a \
 	   clangSema.a clangAnalysis.a clangRewriteFrontend.a clangRewrite.a \

clang-tools-extra/test/clang-tidy/modernize-pass-by-value.cpp

@@ -0,0 +1,197 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-pass-by-value %t
+// REQUIRES: shell
+
+// CHECK-FIXES: #include <utility>
+
+namespace {
+// POD types are trivially move constructible.
+struct Movable {
+  int a, b, c;
+};
+
+struct NotMovable {
+  NotMovable() = default;
+  NotMovable(const NotMovable &) = default;
+  NotMovable(NotMovable &&) = delete;
+  int a, b, c;
+};
+}
+
+struct A {
+  A(const Movable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value]
+  // CHECK-FIXES: A(Movable M) : M(std::move(M)) {}
+  Movable M;
+};
+
+// Test that we aren't modifying other things than a parameter.
+Movable GlobalObj;
+struct B {
+  B(const Movable &M) : M(GlobalObj) {}
+  // CHECK-FIXES: B(const Movable &M) : M(GlobalObj) {}
+  Movable M;
+};
+
+// Test that a parameter with more than one reference to it won't be changed.
+struct C {
+  // Tests extra-reference in body.
+  C(const Movable &M) : M(M) { this->i = M.a; }
+  // CHECK-FIXES: C(const Movable &M) : M(M) { this->i = M.a; }
+
+  // Tests extra-reference in init-list.
+  C(const Movable &M, int) : M(M), i(M.a) {}
+  // CHECK-FIXES: C(const Movable &M, int) : M(M), i(M.a) {}
+  Movable M;
+  int i;
+};
+
+// Test that both declaration and definition are updated.
+struct D {
+  D(const Movable &M);
+  // CHECK-FIXES: D(Movable M);
+  Movable M;
+};
+D::D(const Movable &M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: D::D(Movable M) : M(std::move(M)) {}
+
+// Test with default parameter.
+struct E {
+  E(const Movable &M = Movable()) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: E(Movable M = Movable()) : M(std::move(M)) {}
+  Movable M;
+};
+
+// Test with object that can't be moved.
+struct F {
+  F(const NotMovable &NM) : NM(NM) {}
+  // CHECK-FIXES: F(const NotMovable &NM) : NM(NM) {}
+  NotMovable NM;
+};
+
+// Test unnamed parameter in declaration.
+struct G {
+  G(const Movable &);
+  // CHECK-FIXES: G(Movable );
+  Movable M;
+};
+G::G(const Movable &M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: G::G(Movable M) : M(std::move(M)) {}
+
+// Test parameter with and without qualifier.
+namespace ns_H {
+typedef ::Movable HMovable;
+}
+struct H {
+  H(const ns_H::HMovable &M);
+  // CHECK-FIXES: H(ns_H::HMovable M);
+  ns_H::HMovable M;
+};
+using namespace ns_H;
+H::H(const HMovable &M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: H(HMovable M) : M(std::move(M)) {}
+
+// Try messing up with macros.
+#define MOVABLE_PARAM(Name) const Movable & Name
+// CHECK-FIXES: #define MOVABLE_PARAM(Name) const Movable & Name
+struct I {
+  I(MOVABLE_PARAM(M)) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: I(MOVABLE_PARAM(M)) : M(M) {}
+  Movable M;
+};
+#undef MOVABLE_PARAM
+
+// Test that templates aren't modified.
+template <typename T> struct J {
+  J(const T &M) : M(M) {}
+  // CHECK-FIXES: J(const T &M) : M(M) {}
+  T M;
+};
+J<Movable> j1(Movable());
+J<NotMovable> j2(NotMovable());
+
+struct K_Movable {
+  K_Movable() = default;
+  K_Movable(const K_Movable &) = default;
+  K_Movable(K_Movable &&o) { dummy = o.dummy; }
+  int dummy;
+};
+
+// Test with movable type with an user defined move constructor.
+struct K {
+  K(const K_Movable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: K(K_Movable M) : M(std::move(M)) {}
+  K_Movable M;
+};
+
+template <typename T> struct L {
+  L(const Movable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: L(Movable M) : M(std::move(M)) {}
+  Movable M;
+};
+L<int> l(Movable());
+
+// Test with a non-instantiated template class.
+template <typename T> struct N {
+  N(const Movable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: N(Movable M) : M(std::move(M)) {}
+
+  Movable M;
+  T A;
+};
+
+// Test with value parameter.
+struct O {
+  O(Movable M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: O(Movable M) : M(std::move(M)) {}
+  Movable M;
+};
+
+// Test with a const-value parameter.
+struct P {
+  P(const Movable M) : M(M) {}
+  // CHECK-FIXES: P(const Movable M) : M(M) {}
+  Movable M;
+};
+
+// Test with multiples parameters where some need to be changed and some don't.
+// need to.
+struct Q {
+  Q(const Movable &A, const Movable &B, const Movable &C, double D)
+      : A(A), B(B), C(C), D(D) {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: pass by value and use std::move
+  // CHECK-MESSAGES: :[[@LINE-3]]:41: warning: pass by value and use std::move
+  // CHECK-FIXES:      Q(const Movable &A, Movable B, Movable C, double D)
+  // CHECK-FIXES:     : A(A), B(std::move(B)), C(std::move(C)), D(D) {}
+  const Movable &A;
+  Movable B;
+  Movable C;
+  double D;
+};
+
+// Test that value-parameters with a nested name specifier are left as-is.
+namespace ns_R {
+typedef ::Movable RMovable;
+}
+struct R {
+  R(ns_R::RMovable M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
+  // CHECK-FIXES: R(ns_R::RMovable M) : M(std::move(M)) {}
+  ns_R::RMovable M;
+};
+
+// Test with rvalue parameter.
+struct S {
+  S(Movable &&M) : M(M) {}
+  // CHECK-FIXES: S(Movable &&M) : M(M) {}
+  Movable M;
+};
+