[clang-tidy] Add UnnecessaryCopyInitialization check to new "performance" module in ClangTidy

Summary:
The patch adds a new ClangTidy check that detects when expensive-to-copy types are unnecessarily copy initialized from a const reference that has the same or are larger scope than the copy.

It currently only detects this when the copied variable is const qualified. But this will be extended to non const variables if they are only used in a const fashion.

Reviewers: alexfh

Subscribers: cfe-commits

Patch by Felix Berger!

Differential Revision: http://reviews.llvm.org/D15623


git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@256632 91177308-0d34-0410-b5e6-96231b3b80d8

clang-tidy/CMakeLists.txt

@@ -32,5 +32,6 @@ add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)
+add_subdirectory(performance)
 add_subdirectory(readability)
 add_subdirectory(utils)

clang-tidy/Makefile

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

clang-tidy/performance/CMakeLists.txt

@@ -0,0 +1,14 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyPerformanceModule
+  PerformanceTidyModule.cpp
+  UnnecessaryCopyInitialization.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tidy/performance/PerformanceTidyModule.cpp

@@ -0,0 +1,39 @@
+//===--- PeformanceTidyModule.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 "UnnecessaryCopyInitialization.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+class PerformanceModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
+        "performance-unnecessary-copy-initialization");
+  }
+};
+
+// Register the PerformanceModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<PerformanceModule>
+    X("performance-module", "Adds performance checks.");
+
+} // namespace performance
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the PerformanceModule.
+volatile int PerformanceModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

@@ -0,0 +1,73 @@
+//===--- UnnecessaryCopyInitialization.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 "UnnecessaryCopyInitialization.h"
+
+#include "../utils/LexerUtils.h"
+#include "../utils/Matchers.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+using namespace ::clang::ast_matchers;
+
+namespace {
+AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); }
+AST_MATCHER(QualType, isPointerType) { return Node->isPointerType(); }
+} // namespace
+
+void UnnecessaryCopyInitialization::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  auto ConstReference = referenceType(pointee(qualType(isConstQualified())));
+  auto ConstOrConstReference =
+      allOf(anyOf(ConstReference, isConstQualified()),
+            unless(allOf(isPointerType(), unless(pointerType(pointee(qualType(
+                                              isConstQualified())))))));
+  // Match method call expressions where the this argument is a const
+  // type or const reference. This returned const reference is highly likely to
+  // outlive the local const reference of the variable being declared.
+  // The assumption is that the const reference being returned either points
+  // to a global static variable or to a member of the called object.
+  auto ConstRefReturningMethodCallOfConstParam = cxxMemberCallExpr(
+      callee(cxxMethodDecl(returns(ConstReference))),
+      on(declRefExpr(to(varDecl(hasType(qualType(ConstOrConstReference)))))));
+  auto ConstRefReturningFunctionCall =
+      callExpr(callee(functionDecl(returns(ConstReference))),
+               unless(callee(cxxMethodDecl())));
+  Finder->addMatcher(
+      varDecl(
+          isLocalVarDecl(), hasType(isConstQualified()),
+          hasType(matchers::isExpensiveToCopy()),
+          hasInitializer(cxxConstructExpr(
+              hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
+              hasArgument(0, anyOf(ConstRefReturningFunctionCall,
+                                   ConstRefReturningMethodCallOfConstParam)))))
+          .bind("varDecl"),
+      this);
+}
+
+void UnnecessaryCopyInitialization::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("varDecl");
+  SourceLocation AmpLocation = Var->getLocation();
+  auto Token = lexer_utils::getPreviousNonCommentToken(*Result.Context,
+                                                       Var->getLocation());
+  if (!Token.is(tok::unknown)) {
+    AmpLocation = Token.getLocation().getLocWithOffset(Token.getLength());
+  }
+  diag(Var->getLocation(),
+       "the const qualified variable '%0' is copy-constructed from a "
+       "const reference; consider making it a const reference")
+      << Var->getName() << FixItHint::CreateInsertion(AmpLocation, "&");
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang

clang-tidy/performance/UnnecessaryCopyInitialization.h

@@ -0,0 +1,38 @@
+//===--- UnnecessaryCopyInitialization.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_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+// A check that detects const local variable declarations that are copy
+// initialized with the const reference of a function call or the const
+// reference of a method call whose object is guaranteed to outlive the
+// variable's scope and suggests to use a const reference.
+//
+// The check currently only understands a subset of variables that are
+// guaranteed to outlive the const reference returned, namely: const variables,
+// const references, and const pointers to const.
+class UnnecessaryCopyInitialization : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H

clang-tidy/tool/CMakeLists.txt

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

clang-tidy/tool/ClangTidyMain.cpp

@@ -377,6 +377,11 @@ extern volatile int ModernizeModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ModernizeModuleAnchorDestination =
     ModernizeModuleAnchorSource;
 
+// This anchor is used to force the linker to link the PerformanceModule.
+extern volatile int PerformanceModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =
+    PerformanceModuleAnchorSource;
+
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =

clang-tidy/utils/CMakeLists.txt

@@ -4,6 +4,7 @@ add_clang_library(clangTidyUtils
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
+  LexerUtils.cpp
   TypeTraits.cpp
 
   LINK_LIBS

clang-tidy/utils/LexerUtils.cpp

@@ -0,0 +1,39 @@
+//===--- LexerUtils.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 "LexerUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace lexer_utils {
+
+Token getPreviousNonCommentToken(const ASTContext &Context,
+                                 SourceLocation Location) {
+  const auto &SourceManager = Context.getSourceManager();
+  Token Token;
+  Token.setKind(tok::unknown);
+  Location = Location.getLocWithOffset(-1);
+  auto StartOfFile =
+      SourceManager.getLocForStartOfFile(SourceManager.getFileID(Location));
+  while (Location != StartOfFile) {
+    Location = Lexer::GetBeginningOfToken(Location, SourceManager,
+                                          Context.getLangOpts());
+    if (!Lexer::getRawToken(Location, Token, SourceManager,
+                            Context.getLangOpts()) &&
+        !Token.is(tok::comment)) {
+      break;
+    }
+    Location = Location.getLocWithOffset(-1);
+  }
+  return Token;
+}
+
+} // namespace lexer_utils
+} // namespace tidy
+} // namespace clang

clang-tidy/utils/LexerUtils.h

@@ -0,0 +1,29 @@
+//===--- LexerUtils.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_UTILS_LEXER_UTILS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace tidy {
+namespace lexer_utils {
+
+// Returns previous non-comment token skipping over any comment text or
+// tok::unknown if not found.
+Token getPreviousNonCommentToken(const ASTContext &Context,
+                                 SourceLocation Location);
+
+} // namespace lexer_utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

test/clang-tidy/performance-unnecessary-copy-initialization.cpp

@@ -0,0 +1,152 @@
+// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+
+struct ExpensiveToCopyType {
+  ExpensiveToCopyType() {}
+  virtual ~ExpensiveToCopyType() {}
+  const ExpensiveToCopyType &reference() const { return *this; }
+};
+
+struct TrivialToCopyType {
+  const TrivialToCopyType &reference() const { return *this; }
+};
+
+const ExpensiveToCopyType &ExpensiveTypeReference() {
+  static const ExpensiveToCopyType *Type = new ExpensiveToCopyType();
+  return *Type;
+}
+
+const TrivialToCopyType &TrivialTypeReference() {
+  static const TrivialToCopyType *Type = new TrivialToCopyType();
+  return *Type;
+}
+
+void PositiveFunctionCall() {
+  const auto AutoAssigned = ExpensiveTypeReference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
+  // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference();
+  const auto AutoCopyConstructed(ExpensiveTypeReference());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference());
+  const ExpensiveToCopyType VarAssigned = ExpensiveTypeReference();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES:   const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference();
+  const ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference());
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference());
+}
+
+void PositiveMethodCallConstReferenceParam(const ExpensiveToCopyType &Obj) {
+  const auto AutoAssigned = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
+  const auto AutoCopyConstructed(Obj.reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
+  const ExpensiveToCopyType VarAssigned = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
+}
+
+void PositiveMethodCallConstParam(const ExpensiveToCopyType Obj) {
+  const auto AutoAssigned = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
+  const auto AutoCopyConstructed(Obj.reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
+  const ExpensiveToCopyType VarAssigned = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
+}
+
+void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) {
+  const auto AutoAssigned = Obj->reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoAssigned = Obj->reference();
+  const auto AutoCopyConstructed(Obj->reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& AutoCopyConstructed(Obj->reference());
+  const ExpensiveToCopyType VarAssigned = Obj->reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj->reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj->reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj->reference());
+}
+
+void PositiveLocalConstValue() {
+  const ExpensiveToCopyType Obj;
+  const auto UnnecessaryCopy = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Obj.reference();
+}
+
+void PositiveLocalConstRef() {
+  const ExpensiveToCopyType Obj;
+  const ExpensiveToCopyType &ConstReference = Obj.reference();
+  const auto UnnecessaryCopy = ConstReference.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& UnnecessaryCopy = ConstReference.reference();
+}
+
+void PositiveLocalConstPointer() {
+  const ExpensiveToCopyType Obj;
+  const ExpensiveToCopyType *const ConstPointer = &Obj;
+  const auto UnnecessaryCopy = ConstPointer->reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-FIXES: const auto& UnnecessaryCopy = ConstPointer->reference();
+}
+
+void NegativeFunctionCallTrivialType() {
+  const auto AutoAssigned = TrivialTypeReference();
+  const auto AutoCopyConstructed(TrivialTypeReference());
+  const TrivialToCopyType VarAssigned = TrivialTypeReference();
+  const TrivialToCopyType VarCopyConstructed(TrivialTypeReference());
+}
+
+void NegativeFunctionCallExpensiveTypeNonConstVariable() {
+  auto AutoAssigned = ExpensiveTypeReference();
+  auto AutoCopyConstructed(ExpensiveTypeReference());
+  ExpensiveToCopyType VarAssigned = ExpensiveTypeReference();
+  ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference());
+}
+
+void NegativeMethodCallNonConstRef(ExpensiveToCopyType &Obj) {
+  const auto AutoAssigned = Obj.reference();
+  const auto AutoCopyConstructed(Obj.reference());
+  const ExpensiveToCopyType VarAssigned = Obj.reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+}
+
+void NegativeMethodCallNonConst(ExpensiveToCopyType Obj) {
+  const auto AutoAssigned = Obj.reference();
+  const auto AutoCopyConstructed(Obj.reference());
+  const ExpensiveToCopyType VarAssigned = Obj.reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+}
+
+void NegativeMethodCallNonConstPointer(ExpensiveToCopyType *const Obj) {
+  const auto AutoAssigned = Obj->reference();
+  const auto AutoCopyConstructed(Obj->reference());
+  const ExpensiveToCopyType VarAssigned = Obj->reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj->reference());
+}
+
+void NegativeObjIsNotParam() {
+  ExpensiveToCopyType Obj;
+  const auto AutoAssigned = Obj.reference();
+  const auto AutoCopyConstructed(Obj.reference());
+  const ExpensiveToCopyType VarAssigned = Obj.reference();
+  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+}
+
+struct NegativeConstructor {
+  NegativeConstructor(const ExpensiveToCopyType &Obj) : Obj(Obj) {}
+  ExpensiveToCopyType Obj;
+};