Add UnnecessaryCopyInitialization check to new "performance" module in ClangTidy

[flx]

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.

[alexfh]

Thank you for working on this!

A few minor comments.

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

21 ↗

(On Diff #43170)

Maybe move this to clang-tidy/utils/Matchers.h?

24 ↗

(On Diff #43170)

This might be useful elsewhere. Let's move it to clang-tidy/utils/LexerUtils.h / .cpp.

clang-tidy/performance/UnnecessaryCopyInitialization.h

16 ↗

(On Diff #43170)

This should also be in the performance namespace. The add_new_check.py script has been fixed in the meantime.

[alexfh]

Looks good! I'll commit the patch for you.

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

 
 add_subdirectory(tool)
 add_subdirectory(cert)
 add_subdirectory(llvm)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)
+add_subdirectory(performance)
 add_subdirectory(readability)
 add_subdirectory(utils)

clang-tools-extra/trunk/clang-tidy/Makefile

 ##===- tools/extra/clang-tidy/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 := 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-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyPerformanceModule
+  PerformanceTidyModule.cpp
+  UnnecessaryCopyInitialization.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp

+//===--- 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-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h

+//===--- 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-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

+//===--- 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-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt

target_link_libraries(clang-tidy
   clangBasic
   clangTidy
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
+  clangTidyPerformanceModule
   clangTidyReadabilityModule
   clangTooling
   )
 
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
 install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
 install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)

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

 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 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 =
     ReadabilityModuleAnchorSource;
 
 } // namespace tidy
 } // namespace clang
 
 int main(int argc, const char **argv) {
   return clang::tidy::clangTidyMain(argc, argv);
 }

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyUtils
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
+  LexerUtils.cpp
   TypeTraits.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/utils/LexerUtils.h

+//===--- 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

clang-tools-extra/trunk/clang-tidy/utils/LexerUtils.cpp

+//===--- 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-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp

+// 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;
+};