[clang-tidy] Add performance-emplace-into-containers

[vsk]

Summary

Introduce a check which suggests when it might be helpful to use "emplace" methods. The initial version only works with std::vector and push_back (e.g V.push_back(T(1, 2)) -> V.emplace_back(1, 2)). It could be extended to work with other containers and methods.

[vsk]

• Fix handling of push_back(X(/* comment */)).
• Don't try to emit a warning when the callee comes from a macro expansion.
• Get rid of a weird/wrong comment about checking for "default constructors".

As a side note, I'd appreciate advice on how to apply this check to the llvm codebase (via some kind of cmake invocation?) and gather numbers.

[vsk]

• Fix the diagnostic message. Suggested by Erik Pilkington.

[Eugene.Zelenko]

See also D20964. I think modernize is better place for such check.

[sbenza]

Missing the .rst file.
Did you use the check_clang_tidy.py script to create the template?

clang-tidy/performance/EmplaceCheck.cpp

25

This change can potentially break exception guarantees of the code.
For example, push_back can throw before the object is constructed leaking the arguments.
We should at least say something about it in the docs.

26

Just say functionDecl(hasName("std::vector::push_back")) and you can remove the type check.

84

It's usually preferable, and sometimes easier to write, to do erases instead of replacements.
For example, you could simply erase X( and ).
Something like (Cons->getLogStart(), ArgsR.getBegin()) and (Args.getEnd(), Args.getEnd())

88

What you want is getTokenRange(Call->getCallee()->getSourceRange())
No need to mess with the locations and offsets yourself.

clang-tidy/performance/EmplaceCheck.h

18

Needs a comment

19

What's the scope?
Is it just vector::push_back/emplace_back?
Or is it going to be expanded to other map::insert, deque::push_front, etc.

test/clang-tidy/performance-emplace-into-containers.cpp

26

We have to make sure the arguments are compatible with perfect forwarding.
This means no brace init lists and no NULL.
Eg:

v.push_back(Y({}));  // {} can't be passed to perfect forwarding
v.push_back(Y(NULL));  // NULL can't be passed to perfect forwarding

28

At least one of the cases should match the whole exact message, to make sure it is correct.

42

We should also test implicit conversions.
Eg:

v1.push_back(0);

46

Why not?

50

We need tests for copy/move construction.
Eg:

v1.push_back(x);  // should not be changed
v1.push_back(FunctionThatReturnsX());  // should not be changed

[aaron.ballman]

clang-tidy/performance/EmplaceCheck.cpp

26

Should use "::std::vector::push_back" just to be sure it doesn't explode when evil people have:

namespace frobble {
namespace std {
template <typename T>
class vector {
  void push_back(const T&);
};
}
}

38

I think this goes against our coding standards and should be return StringRef();.

[Eugene.Zelenko]

Since D20964 was committed, I think we should close this.

[Prazek]

In D21185#464517, @Eugene.Zelenko wrote:

Since D20964 was committed, I think we should close this.

Yep, I think the best idea is to take all the goodies from this check and add it to modernize-use-emplace

[vsk]

Thanks for the valuable feedback! I'll keep all of it in mind when writing checks in the future.

I'm closing this revision since D20964 is in.

clang-tidy/performance/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
+  EmplaceCheck.cpp
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitCastInLoopCheck.cpp
   PerformanceTidyModule.cpp
   UnnecessaryCopyInitialization.cpp
   UnnecessaryValueParamCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/performance/EmplaceCheck.h

+//===--- EmplaceCheck.h - clang-tidy --------------------------------------===//
+//
+//                     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_EMPLACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EMPLACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+class EmplaceCheck : public ClangTidyCheck {
+public:
+  EmplaceCheck(StringRef Name, ClangTidyContext *Context);
+  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_EMPLACE_H

clang-tidy/performance/EmplaceCheck.cpp

+//===--- EmplaceCheck.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 "EmplaceCheck.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+using namespace ast_matchers;
+
+EmplaceCheck::EmplaceCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+void EmplaceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxMemberCallExpr(
+          on(expr(hasType(cxxRecordDecl(hasName("std::vector"))))),
+          callee(functionDecl(hasName("push_back"))),
+          hasArgument(0, cxxConstructExpr().bind("construct_expr")))
+          .bind("call"),
+      this);
+}
+
+namespace {
+
+/// \brief Get a StringRef representing a SourceRange.
+StringRef getAsString(const SourceManager &SM, const LangOptions &LO,
+                      SourceRange SR) {
+  if (SR.isInvalid())
+    return {};
+  return Lexer::getSourceText(Lexer::getAsCharRange(SR, SM, LO), SM, LO);
+}
+
+/// \brief Return the precise end location for the given token.
+///
+/// Use the offset to the last character in the token, instead of the offset
+/// to the first character after the token.
+SourceLocation getPreciseTokenEnd(const SourceManager &SM,
+                                  const LangOptions &LO, SourceLocation Loc) {
+  unsigned TokLen = Lexer::MeasureTokenLength(SM.getSpellingLoc(Loc), SM, LO);
+  return Loc.getLocWithOffset(TokLen - 1);
+}
+
+/// \brief Return the range containing the given token.
+SourceRange getPreciseTokenRange(const SourceManager &SM, const LangOptions &LO,
+                                 SourceLocation Loc) {
+  return {Loc, getPreciseTokenEnd(SM, LO, Loc)};
+}
+
+} // end anonymous namespace
+
+void EmplaceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const auto *Cons = Result.Nodes.getNodeAs<CXXConstructExpr>("construct_expr");
+
+  // We need this range to emit a fixit.
+  SourceRange ArgsR = Cons->getParenOrBraceRange();
+  if (ArgsR.isInvalid())
+    return;
+
+  // Don't try to mess with callees that come from expansions.
+  SourceLocation CalleeStartLoc = Call->getExprLoc();
+  if (CalleeStartLoc.isMacroID())
+    return;
+
+  // Don't try to forward initializer lists.
+  if (Cons->isListInitialization() || Cons->isStdInitListInitialization())
+    return;
+
+  const SourceManager &SM = *Result.SourceManager;
+  const LangOptions &LO = Result.Context->getLangOpts();
+
+  // Skip just past the left parenthesis in '(', ..., ')'.
+  ArgsR.setBegin(Lexer::getLocForEndOfToken(ArgsR.getBegin(), 0, SM, LO));
+
+  StringRef ArgsStr = getAsString(SM, LO, ArgsR);
+  if (!ArgsStr.data())
+    return;
+
+  SourceRange CalleeR = getPreciseTokenRange(SM, LO, CalleeStartLoc);
+  SourceRange ConstructR = Cons->getSourceRange();
+
+  diag(CalleeStartLoc, "Consider constructing the vector element in-place")
+      << FixItHint::CreateReplacement(CalleeR, "emplace_back")
+      << FixItHint::CreateReplacement(ConstructR, ArgsStr);
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang

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 "EmplaceCheck.h"
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitCastInLoopCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<EmplaceCheck>(
+        "performance-emplace-into-containers");
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "performance-faster-string-find");
     CheckFactories.registerCheck<ForRangeCopyCheck>(
         "performance-for-range-copy");
     CheckFactories.registerCheck<ImplicitCastInLoopCheck>(
         "performance-implicit-cast-in-loop");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");

test/clang-tidy/performance-emplace-into-containers.cpp

+// RUN: %check_clang_tidy %s performance-emplace-into-containers %t -- -- -std=c++11
+
+namespace std {
+
+template <typename T>
+struct vector {
+  void push_back(const T &x);
+  void push_back(T &&x);
+
+  template <class... Args>
+  void emplace_back(Args &&... args);
+};
+
+} // std
+
+struct X {
+  int x;
+  X() : x(0) {}
+  X(int x) : x(x) {}
+  X(int x, int y) : x(x + y) {}
+  X(const X &Obj) : x(Obj.x) {}
+};
+
+void f1() {
+  std::vector<X> v1;
+
+  v1.push_back(X());
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: Consider constructing {{.*}}
+  // CHECK-FIXES: v1.emplace_back()
+
+  v1.push_back(X(/*a*/));
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: Consider constructing {{.*}}
+  // CHECK-FIXES: v1.emplace_back(/*a*/)
+
+  v1.push_back(X(0, 0));
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: Consider constructing {{.*}}
+  // CHECK-FIXES: v1.emplace_back(0, 0)
+
+  v1/*a*/./*b*/push_back(/*c*/X(/*d*/0,/*e*/0/*f*/)/*g*/)/*h*/ ;
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: Consider constructing {{.*}}
+  // CHECK-FIXES: v1/*a*/./*b*/emplace_back(/*c*//*d*/0,/*e*/0/*f*//*g*/)/*h*/ ;
+
+  // Do not try to deal with initializer lists.
+  v1.push_back({0, 0});
+
+  // Do not try to deal with functional casts. FIXME?
+  X x{0, 0};
+  v1.push_back(X(x));
+  v1.push_back(X(0));
+
+  // Do not try to deal with weird expansions.
+#define M1 X(0, 0)
+#define M2 push_back
+  v1.push_back(M1);
+  v1.M2(X(0, 0));
+  v1.M2(M1);
+}