[clang-tidy] Add modernize-use-emplace

Summary: Add check that replaces call of push_back to emplace_back

Reviewers: hokein

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

llvm-svn: 273275

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

@@ -17,6 +17,7 @@ add_clang_library(clangTidyModernizeModule
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultCheck.cpp
+  UseEmplaceCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
 

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

@@ -23,6 +23,7 @@
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultCheck.h"
+#include "UseEmplaceCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 
@@ -54,6 +55,7 @@ public:
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
+    CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
   }

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

@@ -0,0 +1,104 @@
+//===--- UseEmplaceCheck.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 "UseEmplaceCheck.h"
+#include "../utils/Matchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  // FIXME: Bunch of functionality that could be easily added:
+  // + add handling of `push_front` for std::forward_list, std::list
+  // and std::deque.
+  // + add handling of `push` for std::stack, std::queue, std::priority_queue
+  // + add handling of `insert` for stl associative container, but be careful
+  // because this requires special treatment (it could cause performance
+  // regression)
+  // + match for emplace calls that should be replaced with insertion
+  // + match for make_pair calls.
+  auto callPushBack = cxxMemberCallExpr(
+      hasDeclaration(functionDecl(hasName("push_back"))),
+      on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
+                                          "std::list", "std::deque")))));
+
+  // We can't replace push_backs of smart pointer because
+  // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
+  // passed pointer because smart pointer won't be constructed
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+      ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
+                         "std::weak_ptr"))));
+
+  // Bitfields binds only to consts and emplace_back take it by universal ref.
+  auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(
+      memberExpr(hasDeclaration(fieldDecl(matchers::isBitfield())))));
+
+  // We could have leak of resource.
+  auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr()));
+  auto constructingDerived =
+      hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
+
+  auto hasInitList = has(ignoringParenImpCasts(initListExpr()));
+  auto soughtConstructExpr =
+      cxxConstructExpr(
+          unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
+                       newExprAsArgument, constructingDerived,
+                       has(materializeTemporaryExpr(hasInitList)))))
+          .bind("ctor");
+  auto hasConstructExpr = has(ignoringParenImpCasts(soughtConstructExpr));
+
+  auto ctorAsArgument = materializeTemporaryExpr(
+      anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr))));
+
+  Finder->addMatcher(
+      cxxMemberCallExpr(callPushBack, has(ctorAsArgument)).bind("call"), this);
+}
+
+void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const auto *InnerCtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
+
+  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
+      Call->getExprLoc(), Call->getArg(0)->getExprLoc());
+
+  auto Diag = diag(Call->getExprLoc(), "use emplace_back instead of push_back");
+
+  if (FunctionNameSourceRange.getBegin().isMacroID())
+    return;
+
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
+                                       "emplace_back(");
+
+  auto CallParensRange = InnerCtorCall->getParenOrBraceRange();
+
+  // Finish if there is no explicit constructor call.
+  if (CallParensRange.getBegin().isInvalid())
+    return;
+
+  // Range for constructor name and opening brace.
+  auto CtorCallSourceRange = CharSourceRange::getCharRange(
+      InnerCtorCall->getExprLoc(),
+      CallParensRange.getBegin().getLocWithOffset(1));
+
+  Diag << FixItHint::CreateRemoval(CtorCallSourceRange)
+       << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+              CallParensRange.getEnd(),
+              CallParensRange.getEnd().getLocWithOffset(1)));
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,38 @@
+//===--- UseEmplaceCheck.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_USE_EMPLACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// This check looks for cases when inserting new element into std::vector but
+/// the element is constructed temporarily.
+/// It replaces those calls for emplace_back of arguments passed to
+/// constructor of temporary object.
+///`
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
+class UseEmplaceCheck : public ClangTidyCheck {
+public:
+  UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H

clang-tools-extra/clang-tidy/utils/Matchers.h

@@ -45,6 +45,8 @@ AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) {
       Node, Finder->getASTContext());
 }
 
+AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
+
 } // namespace matchers
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

@@ -220,6 +220,11 @@ identified.  The improvements since the 3.8 release include:
 
   Finds integer literals which are cast to ``bool``.
 
+- New `modernize-use-emplace
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html>`_ check
+
+  Finds calls that could be changed to emplace.
+
 - New `performance-faster-string-find
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-faster-string-find.html>`_ check
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -104,6 +104,7 @@ Clang-Tidy Checks
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default
+   modernize-use-emplace
    modernize-use-nullptr
    modernize-use-override
    performance-faster-string-find

clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst

@@ -0,0 +1,69 @@
+.. title:: clang-tidy - modernize-use-emplace
+
+modernize-use-emplace
+=====================
+
+This check looks for cases when inserting new element into an STL
+container (``std::vector``, ``std::deque``, ``std::list``) or ``llvm::SmallVector``
+but the element is constructed temporarily.
+
+Before:
+
+.. code:: c++
+
+	std::vector<MyClass> v;
+	v.push_back(MyClass(21, 37));
+
+	std::vector<std::pair<int,int> > w;
+	w.push_back(std::make_pair(21, 37));
+
+After:
+
+.. code:: c++
+
+	std::vector<MyClass> v;
+	v.emplace_back(21, 37);
+
+	std::vector<std::pair<int,int> > w;
+	v.emplace_back(21, 37);
+
+The other situation is when we pass arguments that will be converted to a type
+inside a container.
+
+Before:
+
+.. code:: c++
+
+	std::vector<boost::optional<std::string> > v;
+	v.push_back("abc");
+
+After:
+
+.. code:: c++
+
+	std::vector<boost::optional<std::string> > v;
+	v.emplace_back("abc");
+
+
+In this case the calls of ``push_back`` won't be replaced.
+
+.. code:: c++
+	std::vector<std::unique_ptr<int> > v;
+	v.push_back(new int(5));
+	auto *ptr = int;
+	v.push_back(ptr);
+
+This is because replacing it with ``emplace_back`` could cause a leak of this
+pointer if ``emplace_back`` would throw exception before emplacement
+(e.g. not enough memory to add new element).
+
+For more info read item 42 - "Consider emplacement instead of insertion."
+of Scott Meyers Efective Modern C++.
+
+Check also fires if any argument of constructor call would be:
+- bitfield (bitfields can't bind to rvalue/universal reference)
+- ``new`` expression (to avoid leak)
+or if the argument would be converted via derived-to-base cast.
+
+This check requires C++11 of higher to run.
+

clang-tools-extra/test/clang-tidy/modernize-use-emplace.cpp

@@ -0,0 +1,338 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template <typename T>
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template <typename... Args>
+  void emplace_back(Args &&... args){};
+};
+template <typename T>
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template <typename... Args>
+  void emplace_back(Args &&... args){};
+};
+
+template <typename T>
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template <typename... Args>
+  void emplace_back(Args &&... args){};
+};
+
+template <typename T1, typename T2>
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template <class U1, class U2>
+  pair(const pair<U1, U2> &p){};
+  template <class U1, class U2>
+  pair(pair<U1, U2> &&p){};
+};
+
+template <typename T1, typename T2>
+pair<T1, T2> make_pair(T1, T2) {
+  return pair<T1, T2>();
+};
+
+template <typename T>
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector<int> v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  v.push_back(42.0);
+  int z;
+  v.push_back(z);
+}
+
+struct Something {
+  Something(int a, int b = 41) {}
+  Something() {}
+  void push_back(Something);
+};
+
+struct Convertable {
+  operator Something() { return Something{}; }
+};
+
+struct Zoz {
+  Zoz(Something, int = 42) {}
+};
+
+Zoz getZoz(Something s) { return Zoz(s); }
+
+void test_Something() {
+  std::vector<Something> v;
+
+  v.push_back(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(42);
+
+  Something temporary(42, 42);
+  temporary.push_back(temporary);
+  v.push_back(temporary);
+
+  v.push_back(Convertable());
+  v.push_back(Convertable{});
+  Convertable s;
+  v.push_back(s);
+}
+
+void test2() {
+  std::vector<Zoz> v;
+  v.push_back(Zoz(Something(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(Something(21, 37));
+
+  v.push_back(Zoz(Something(21, 37), 42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(Something(21, 37), 42);
+
+  v.push_back(getZoz(Something(1, 2)));
+}
+
+void testPair() {
+  std::vector<std::pair<int, int>> v;
+  v.push_back(std::pair<int, int>(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector<std::pair<Something, Zoz>> v2;
+  v2.push_back(std::pair<Something, Zoz>(Something(42, 42), Zoz(Something(21, 37))));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
+}
+
+struct Base {
+  Base(int, int *, int = 42);
+};
+
+struct Derived : Base {
+  Derived(int *, Something) : Base(42, nullptr) {}
+};
+
+void testDerived() {
+  std::vector<Base> v;
+  v.push_back(Derived(nullptr, Something{}));
+}
+
+void testNewExpr() {
+  std::vector<Derived> v;
+  v.push_back(Derived(new int, Something{}));
+}
+
+void testSpaces() {
+  std::vector<Something> v;
+
+  // clang-format off
+
+  v.push_back(Something(1, //arg1
+                2 // arg2
+               ) // Something
+              );
+  // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, //arg1
+  // CHECK-FIXES:                2 // arg2
+  // CHECK-FIXES:                  // Something
+  // CHECK-FIXES:                );
+
+  v.push_back(    Something   (1, 2)    );
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+
+  v.push_back(    Something   {1, 2}    );
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+
+  v.push_back(  Something {}    );
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(   );
+
+  v.push_back(
+             Something(1, 2)    );
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+
+  std::vector<Base> v2;
+  v2.push_back(
+    Base(42, nullptr));
+  // CHECK-MESSAGES: :[[@LINE-2]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back(42, nullptr);
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector<int *> v;
+  v.push_back(new int(5));
+
+  std::vector<std::unique_ptr<int>> v2;
+  v2.push_back(new int(42));
+  // This call can't be replaced with emplace_back.
+  // If emplacement will fail (not enough memory to add to vector)
+  // we will have leak of int because unique_ptr won't be constructed
+  // (and destructed) as in push_back case.
+
+  auto *ptr = new int;
+  v2.push_back(ptr);
+  // Same here
+}
+
+void testMakePair() {
+  std::vector<std::pair<int, int>> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+
+  // FIXME: This is not a bug, but call of make_pair should be removed in the
+  // future. This one matches because the return type of make_pair is different
+  // than the pair itself.
+  v.push_back(std::make_pair(42LL, 13));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+}
+
+void testOtherCointainers() {
+  std::list<Something> l;
+  l.push_back(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: l.emplace_back(42, 41);
+
+  std::deque<Something> d;
+  d.push_back(Something(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: d.emplace_back(42);
+}
+
+class IntWrapper {
+public:
+  IntWrapper(int x) : value(x) {}
+  IntWrapper operator+(const IntWrapper other) const {
+    return IntWrapper(value + other.value);
+  }
+
+private:
+  int value;
+};
+
+void testMultipleOpsInPushBack() {
+  std::vector<IntWrapper> v;
+  v.push_back(IntWrapper(42) + IntWrapper(27));
+}
+
+// Macro tests.
+#define PUSH_BACK_WHOLE(c, x) c.push_back(x)
+#define PUSH_BACK_NAME push_back
+#define PUSH_BACK_ARG(x) (x)
+#define SOME_OBJ Something(10)
+#define MILLION 3
+#define SOME_WEIRD_PUSH(v) v.push_back(Something(
+#define OPEN (
+#define CLOSE )
+void macroTest() {
+  std::vector<Something> v;
+  Something s;
+
+  PUSH_BACK_WHOLE(v, Something(5, 6));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use emplace_back
+
+  v.PUSH_BACK_NAME(Something(5));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+
+  v.push_back PUSH_BACK_ARG(Something(5, 6));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+
+  v.push_back(SOME_OBJ);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+
+  v.push_back(Something(MILLION));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(MILLION);
+
+  // clang-format off
+  v.push_back(  Something OPEN 3 CLOSE  );
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // clang-format on
+  PUSH_BACK_WHOLE(s, Something(1));
+}
+
+struct A {
+  int value1, value2;
+};
+
+struct B {
+  B(A) {}
+};
+
+struct C {
+  int value1, value2, value3;
+};
+
+void testAggregation() {
+  // This should not be noticed or fixed; after the correction, the code won't
+  // compile.
+
+  std::vector<A> v;
+  v.push_back(A({1, 2}));
+
+  std::vector<B> vb;
+  vb.push_back(B({10, 42}));
+}
+
+struct Bitfield {
+  unsigned bitfield : 1;
+  unsigned notBitfield;
+};
+
+void testBitfields() {
+  std::vector<Something> v;
+  Bitfield b;
+  v.push_back(Something(42, b.bitfield));
+  v.push_back(Something(b.bitfield));
+
+  v.push_back(Something(42, b.notBitfield));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(42, b.notBitfield);
+  int var;
+  v.push_back(Something(42, var));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(42, var);
+}