Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Thank you Samuel, that it is valuable review!
I wanted to add the functionality of specifing stuff by options later.

should we revert this patch? It seems that those bugs are not so critical (at 
least on llvm http://reviews.llvm.org/D21507).
Anyway, I will fix it in one week.


Repository:
  rL LLVM

http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:34
@@ +33,3 @@
+  hasDeclaration(functionDecl(hasName("push_back"))),
+  on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
+  "std::list", "std::deque");

We should not hard code this list. Specially not non-standard types like 
llvm::SmallVector.
This should come from an option string.
For example, like we do in performance/FasterStringFindCheck.cpp


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:42
@@ +41,3 @@
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
+ "std::weak_ptr";

These are not the only smart pointers around.
It might be a good idea to make this list also configurable by the user.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:45
@@ +44,3 @@
+
+  // Bitfields binds only to consts and emplace_back take it by universal ref.
+  auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(

There are a few more things that can break here:
  - `NULL` can't be passed through perfect forwarding. Will be deduced as 
`long`.
  - Function references/pointers can't be passed through PF if they are 
overloaded.
  - Class scope static variables that have no definition can't be passed 
through PF because they will be ODR used.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:92
@@ +91,3 @@
+  // Range for constructor name and opening brace.
+  auto CtorCallSourceRange = CharSourceRange::getCharRange(
+  InnerCtorCall->getExprLoc(),

You should avoid using offsets with locations.
For example, you are hardcoding `{` as one character, which might not be true 
in the case of digraphs.

This should be `getTokenRange(InnerCtorCall->getExprLoc(), 
CallParensRange.getBegin())`

Same thing for the other one, it should be:

`CharSourceRange::getTokenRange(CallParensRange.getEnd(), 
CallParensRange.getEnd())`


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h:23
@@ +22,3 @@
+/// constructor of temporary object.
+///`
+/// For the user-facing documentation see:

runaway qoute


Comment at: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h:48
@@ -47,1 +47,3 @@
 
+AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
+

This matcher is generic enough that it should go into main ASTMatchers.h file.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:17
@@ +16,3 @@
+
+   std::vector > w;
+   w.push_back(std::make_pair(21, 37));

Don't add the space between the >>. This is not needed since we are in C++11.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:52
@@ +51,3 @@
+   std::vector v;
+   v.push_back(new int(5));
+   auto *ptr = int;

This doesn't compile. unique_ptr is not implicitly convertible from a pointer.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:53
@@ +52,3 @@
+   v.push_back(new int(5));
+   auto *ptr = int;
+   v.push_back(ptr);

I think you meant `auto* ptr = new int(5)` ?


Comment at: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:76
@@ +75,3 @@
+};
+
+struct Convertable {

You should also try with a type that has a user-defined destructor.
It changes the AST enough to make a difference in many cases.

And you should fix all the std classes above to have user-defined constructors 
too.


Comment at: 
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:209
@@ +208,3 @@
+  std::vector v2;
+  v2.push_back(new int(42));
+  // This call can't be replaced with emplace_back.

This test is broken. `unique_ptr` should not have an implicit conversion 
from T*.


Comment at: 
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:338
@@ +337,2 @@
+  // CHECK-FIXES: v.emplace_back(42, var);
+}

It is also missing tests with templates.
ie: the container being a dependent type, and the value_type being a dependent 
type.
We should not change the code in either of those cases.


Repository:
  rL LLVM

http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL273275: [clang-tidy] Add modernize-use-emplace (authored by 
Prazek).

Changed prior to commit:
  http://reviews.llvm.org/D20964?vs=61214=61381#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20964

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp

Index: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
===
--- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
@@ -45,6 +45,8 @@
   Node, Finder->getASTContext());
 }
 
+AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
+
 } // namespace matchers
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/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 ) {
+  const auto *Call = Result.Nodes.getNodeAs("call");
+  const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
+
+  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
+  Call->getExprLoc(), Call->getArg(0)->getExprLoc());
+
+  auto Diag = diag(Call->getExprLoc(), "use emplace_back instead of push_back");
+
+  if 

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Haojian Wu via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Looks almost good now, a few comments. You'd better await for comments from 
@alexfh or @sbenza before committing.



Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:62
@@ +61,3 @@
+
+Check also fire if any argument of constructor call woule be:
+- bitfield (bitfields can't bind to rvalue/universal reference)

s/fire/fires
s/woule/would


Comment at: test/clang-tidy/modernize-use-emplace.cpp:3
@@ +2,3 @@
+
+namespace std {
+template 

Oops, you are right. That's another non-relevant issue (different 
`emplace_back` behaviors in `vector`, `vector`). I'm 
over-concerned here. 




Comment at: test/clang-tidy/modernize-use-emplace.cpp:317
@@ +316,3 @@
+
+void testAggregation() {
+  // This should not be noticed or fixed; after the correction, the code won't

Looks like the test is kind of duplicated with `testAggregate` in L248, you can 
merge that one into this one.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Changes after running http://reviews.llvm.org/D21507


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 61214.
Prazek added a comment.

Changes after running og llvm code base. Adding llvm::SmallVector was a good 
idea, because it has fired for much more cases and I have found 3 other bugs.
Now check doesn't fire when argument of constructor is bitfield or new 
expression, or when constructor is converted to base class.
I will post diff of changes after running in a minute


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  clang-tidy/utils/Matchers.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,342 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+template 
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector 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 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 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> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(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 v;
+  v.push_back(Derived(nullptr, Something{}));
+}
+
+void testNewExpr() {
+  std::vector v;
+  v.push_back(Derived(new int, Something{}));
+}
+
+void testSpaces() {
+  std::vector 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:   

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a reviewer: sbenza.
Prazek added a comment.

You might be interested.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 8 inline comments as done.
Prazek added a comment.

http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated the summary for this revision.
Prazek updated this revision to Diff 61143.

http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,287 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+template 
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+
+  v.push_back(S(1, //arg1
+2 // arg2
+   ) // S
+  );
+  // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, //arg1
+  // CHECK-FIXES:2 // arg2
+  // CHECK-FIXES:  // S
+  // CHECK-FIXES:);
+
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(   );
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+
+ 

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: test/clang-tidy/modernize-use-emplace.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {

hokein wrote:
> Could you also add the following case in the test?
> ```
> vector v1;
> v1.push_back(1<<10);
> ```
> 
> Make sure the check won't change to `v1.emplace_back(1<<10)`. Since these two 
> statements have completely different semantic. The first one adds `1024` to 
> `v1` while the second one adds a vector with 1024 elements to `v1`.
I think you have made some mistake, because the code that you have showed 
doesn't compile.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Haojian Wu via cfe-commits]

hokein added a comment.

A few comments.



Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59
@@ -57,2 +58,3 @@
 CheckFactories.registerCheck("modernize-use-nullptr");
+CheckFactories.registerCheck("modernize-use-emplace");
 CheckFactories.registerCheck("modernize-use-override");

Alphabetical order.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:37
@@ +36,3 @@
+  // We can't replace push_backs of smart pointer because
+  // if emplacement will fail (f.e. bad_alloc in vector) we will have leak of
+  // passed pointer because smart pointer won't be constructed

remove the `will` here? if emplacement fails, we will ...


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:78
@@ +77,3 @@
+
+  // Range for constructor name and opening brace
+  auto ctorCallSourceRange = CharSourceRange::getCharRange(

missing `.` at the end of sentence.


Comment at: clang-tidy/modernize/UseEmplaceCheck.h:19
@@ +18,3 @@
+
+/// This check look for cases when inserting new element into std::vector but
+/// the element is constructed temporarily.

s/look/looks


Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:6
@@ +5,3 @@
+
+This check look for cases when inserting new element into an STL
+container, but the element is constructed temporarily.

s/look/looks


Comment at: test/clang-tidy/modernize-use-emplace.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {

Could you also add the following case in the test?
```
vector v1;
v1.push_back(1<<10);
```

Make sure the check won't change to `v1.emplace_back(1<<10)`. Since these two 
statements have completely different semantic. The first one adds `1024` to 
`v1` while the second one adds a vector with 1024 elements to `v1`.


Comment at: test/clang-tidy/modernize-use-emplace.cpp:94
@@ +93,3 @@
+  v.push_back(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);

I think you can remove the `{{..}}`.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a reviewer: hokein.
Prazek added a subscriber: hokein.
Prazek added a comment.

@alexfh or @hokein ping


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 60662.
Prazek marked an inline comment as done.
Prazek added a comment.

Runned on LLVM and bug fixed one thing


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,287 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+template 
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+
+  v.push_back(S(1, //arg1
+2 // arg2
+   ) // S
+  );
+  // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, //arg1
+  // CHECK-FIXES:2 // arg2
+  // CHECK-FIXES:  // S
+  // CHECK-FIXES:);
+
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2   );
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(   );
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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() {
+  

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Vedant Kumar via cfe-commits]

vsk added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",

sbarzowski wrote:
> vsk wrote:
> > I agree that blacklisting some smart pointers is not a complete solution, 
> > and that we shouldn't introduce a check which emits false positives.
> > 
> > ISTM it's **only** safe to perform the "push(T(...)) -> emplace(...)" 
> > change if: it's safe to assume that if "emplace(...)" does not successfully 
> > call "T(...)", it's OK for the program to fail/leak/crash. Do we get to 
> > make this assumption ever? Perhaps just in no-exceptions mode?
> hmmm... Maybe it's not that bad.
> 
> Let's look closely at the difference in behaviour.
> 
> with push_back it works like:
> 1) Call the constructor
> 2) Allocate the memory
> 3) Call the copy constructor
> 
> And emplace_back is:
> 1) Allocate the memory
> 2) Call the constructor
> 
> Each of these steps in both scenarios may fail.
> 
> If we call the constructor and it fails, then it's alright. We have the same 
> behaviour in both cases - if it can fail it should do its cleanup. (And 
> failure of copy constructor obviously doesn't bother us).
> 
> So the scenario we have to worry about in only when memory allocation fails. 
> Then with push_back, we'll call constructor and then destructor, while 
> emplace_back doesn't call constructor at all.
> 
> So the real question is: "Is it safe to throw some exception and not to call 
> the constructor at all". And I see only one real-world case it isn't - taking 
> the ownership of the object.
> 
@sbarzowski Right, I think we're in agreement about what the problem is. I just 
don't think there are heuristics or comprehensive blacklists to detect classes 
which have ownership semantics. E.g, consider a FileHandle class which owns a 
file descriptor and is responsible for closing it.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",

vsk wrote:
> I agree that blacklisting some smart pointers is not a complete solution, and 
> that we shouldn't introduce a check which emits false positives.
> 
> ISTM it's **only** safe to perform the "push(T(...)) -> emplace(...)" change 
> if: it's safe to assume that if "emplace(...)" does not successfully call 
> "T(...)", it's OK for the program to fail/leak/crash. Do we get to make this 
> assumption ever? Perhaps just in no-exceptions mode?
hmmm... Maybe it's not that bad.

Let's look closely at the difference in behaviour.

with push_back it works like:
1) Call the constructor
2) Allocate the memory
3) Call the copy constructor

And emplace_back is:
1) Allocate the memory
2) Call the constructor

Each of these steps in both scenarios may fail.

If we call the constructor and it fails, then it's alright. We have the same 
behaviour in both cases - if it can fail it should do its cleanup. (And failure 
of copy constructor obviously doesn't bother us).

So the scenario we have to worry about in only when memory allocation fails. 
Then with push_back, we'll call constructor and then destructor, while 
emplace_back doesn't call constructor at all.

So the real question is: "Is it safe to throw some exception and not to call 
the constructor at all". And I see only one real-world case it isn't - taking 
the ownership of the object.



http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Vedant Kumar via cfe-commits]

vsk added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",

I agree that blacklisting some smart pointers is not a complete solution, and 
that we shouldn't introduce a check which emits false positives.

ISTM it's **only** safe to perform the "push(T(...)) -> emplace(...)" change 
if: it's safe to assume that if "emplace(...)" does not successfully call 
"T(...)", it's OK for the program to fail/leak/crash. Do we get to make this 
assumption ever? Perhaps just in no-exceptions mode?


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Vedant Kumar via cfe-commits]

vsk added a subscriber: vsk.
vsk added a comment.

@Eugene.Zelenko thanks for pointing this out, I had totally missed this patch! 
Once we get this reviewed I'm willing to abandon my version. Some comments 
inline --



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:26
@@ +25,3 @@
+  // + add handling of `push` for std::stack, std::queue, std::priority_queue
+  // + add handling of `insert` for stl associative container, but be cerfull
+  // because this requires special treatment (it could cause performance

cerfull -> careful


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:37
@@ +36,3 @@
+
+  // We can't replace push_backs of smart pointer becase
+  // if emplacement will fail (f.e. bad_alloc in vector) we will have leak of

becase -> because


Comment at: test/clang-tidy/modernize-use-emplace.cpp:147
@@ +146,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+  v.push_back(S   {1, 2});

I don't think this is correct. Comments immediately before/after `S` or 
immediately before/after the second right parenthesis should be preserved. C.f 
the implementation in D21185.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a comment.

There are alternative implementation in http://reviews.llvm.org/D21185. Will be 
good idea to how one which take the best from both :-)


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In http://reviews.llvm.org/D20964#448551, @Eugene.Zelenko wrote:

> In http://reviews.llvm.org/D20964#448525, @Prazek wrote:
>
> > In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:
> >
> > > I think will be good idea to try this check with LLVM STL too.
> >
> >
> > You mean llvm::SmallVector stuff?
>
>
> No, I meant to build example with -stdlib=libc++, -lc++, -lc++abi. Just to 
> make sure, that hasName() is proper matcher.


I runned it on llvm codebase with libstdc++ and it worked perfectly. I don't 
think there should be any change with libc++. I will run it only on small with 
libc++, because it will take too much time to compile whole llvm again.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a comment.

In http://reviews.llvm.org/D20964#448525, @Prazek wrote:

> In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:
>
> > I think will be good idea to try this check with LLVM STL too.
>
>
> You mean llvm::SmallVector stuff?


No, I meant to build example with -stdlib=libc++, -lc++, -lc++abi. Just to make 
sure, that hasName() is proper matcher.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:

> I think will be good idea to try this check with LLVM STL too.


You mean llvm::SmallVector stuff?


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 59580.
Prazek marked 4 inline comments as done.
Prazek added a comment.

- Review fixes


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args) {};
+};
+
+template 
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args) {};
+};
+
+template 
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args) {};
+};
+
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ) {};
+  template 
+  pair(pair &) {};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+}
+
+void testOtherCointainers() {
+  std::list l;
+  l.push_back(S(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: l.emplace_back(42, 41);
+
+  std::deque d;
+  

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

I think will be good idea to try this check with LLVM STL too.



Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:47
@@ +46,3 @@
+
+In this case the calls of push_back won't be replaced.
+

Please highlight push_back with ``.


Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:55
@@ +54,3 @@
+
+This is because replacing it with emplace_back could cause a leak of this
+this pointer, if emplace_back would throw exception before emplacement

Please highlight emplace_back with ``. Same below.


Comment at: test/clang-tidy/modernize-use-emplace.cpp:12
@@ +11,3 @@
+  void emplace_back(Args &&... args);
+};
+template 

Please insert new line.


Comment at: test/clang-tidy/modernize-use-emplace.cpp:78
@@ +77,3 @@
+  operator S() { return S{}; }
+};
+struct Zoz {

Please insert new line.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+  // passed pointer because smart pointer won't be constructed
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(

Prazek wrote:
> sbarzowski wrote:
> > > Look at tests - the same thing happens when
> > 
> > Yeah. I meant looking for `new` in addition to blacklist.
> > 
> > > Not many custom classes take pointer in constructor. 
> > 
> > Obviously depends on codebase, but IMO it's quite common. However usually 
> > this classes aren't copy-constructible (or at least shouldn't be) anyway, 
> > making it impossible to use push_back, so maybe it's not such a big deal.
> > 
> > > If I will look for const pointers, then I will not be able to pass "abc" 
> > > into vector.
> > 
> > I wrote explicitly about only **non**-const pointers.
> It doesn't matter if it is const or not. Disabling any of them would disable 
> some cases where it would be good.
> I have to run it on llvm code base and see what happens
Of course there would be some false negatives. It's close to impossible to know 
for sure if it's safe, so we need some sort of heuristic. My concern is that 
the current "blacklist smart pointers" solution may lead to a lot of false 
positives. And false positives here are quite nasty - very subtle bugs, that 
are unlikely to be caught by the tests or quick code review. So I'm more 
comfortable with false negatives.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 2 inline comments as done.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+  // passed pointer because smart pointer won't be constructed
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(

sbarzowski wrote:
> > Look at tests - the same thing happens when
> 
> Yeah. I meant looking for `new` in addition to blacklist.
> 
> > Not many custom classes take pointer in constructor. 
> 
> Obviously depends on codebase, but IMO it's quite common. However usually 
> this classes aren't copy-constructible (or at least shouldn't be) anyway, 
> making it impossible to use push_back, so maybe it's not such a big deal.
> 
> > If I will look for const pointers, then I will not be able to pass "abc" 
> > into vector.
> 
> I wrote explicitly about only **non**-const pointers.
It doesn't matter if it is const or not. Disabling any of them would disable 
some cases where it would be good.
I have to run it on llvm code base and see what happens


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+  // passed pointer because smart pointer won't be constructed
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(

> Look at tests - the same thing happens when

Yeah. I meant looking for `new` in addition to blacklist.

> Not many custom classes take pointer in constructor. 

Obviously depends on codebase, but IMO it's quite common. However usually this 
classes aren't copy-constructible (or at least shouldn't be) anyway, making it 
impossible to use push_back, so maybe it's not such a big deal.

> If I will look for const pointers, then I will not be able to pass "abc" into 
> vector.

I wrote explicitly about only **non**-const pointers.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 59568.
Prazek added a comment.

- Post review fix


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args);
+};
+template 
+class list {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args);
+};
+
+template 
+class deque {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args);
+};
+
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+}
+
+void testOtherCointainers() {
+  std::list l;
+  l.push_back(S(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: l.emplace_back(42, 41);
+
+  std::deque d;
+  d.push_back(S(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:31
@@ +30,3 @@
+on(hasType(cxxRecordDecl(hasName(VectorName)
+  .bind("call");
+

ok, std::list works for me. I just don't want to spend much time on it right 
now, and I want to be sure it works.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:39
@@ +38,3 @@
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
+ "std::weak_ptr";
+

Look at tests - the same thing happens when

std::vector v;

const auto *p = new int;
v.push_back(p);

Not many custom classes take pointer in constructor. 
If I will look for const pointers, then I will not be able to pass "abc" into 
vector.

So I guess this solution seems to be the best idea. 


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:30
@@ +29,3 @@
+  cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),
+on(hasType(cxxRecordDecl(hasName(VectorName)
+  .bind("call");

Maybe it can work with some more types, with minimal changes... Like maybe 
std::list.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:38
@@ +37,3 @@
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
+ "std::weak_ptr";

This argument really holds for _any_ class taking ownership of the created 
object.

Maybe it would be better to check if we pass a non-const pointer as an argument 
to the constructor, as a pretty good indicator of taking ownership.

Or maybe just look for `new` operator in the expression.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[David Blaikie via cfe-commits]

dblaikie added a subscriber: dblaikie.
dblaikie added a comment.

Could you describe in more detail (ideally in the original patch review 
summary/description) what this transformation does? Under what conditions does 
it suggest emplace, etc.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 2 inline comments as done.
Prazek added a comment.

http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 4 inline comments as done.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:29
@@ +28,3 @@
+  auto callPushBack =
+  cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),
+on(hasType(cxxRecordDecl(hasName(VectorName)

I don't think so. In all checks people use lowerCamelCase, because it looks 
like a real matchers


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 59562.
Prazek added a comment.

Learning how to use arc


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,157 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args);
+};
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+}
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -0,0 +1,60 @@
+.. title:: clang-tidy - modernize-use-emplace
+
+modernize-use-emplace
+=
+
+This check look for cases when inserting new element into an STL
+container, but the element is constructed temporarily.
+
+Before:
+
+.. code:: c++
+
+	std::vector v;
+	v.push_back(MyClass(21, 37));
+
+	std::vector > w;
+	w.push_back(std::make_pair(21, 

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 59561.
Prazek marked an inline comment as done.
Prazek added a comment.

Fixed stuff


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst

Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -3,9 +3,8 @@
 modernize-use-emplace
 =
 
-This check would look for cases when inserting new element into an STL
-container, but the element is constructed temporarily or is constructed just
-to be moved. It would also work with std::pair.
+This check look for cases when inserting new element into an STL
+container, but the element is constructed temporarily.
 
 Before:
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -209,7 +209,7 @@
 - New `modernize-use-emplace
   `_ 
check
 
-  Finds calls that could be change to emplace.
+  Finds calls that could be changed to emplace.
 
 - New `performance-faster-string-find
   
`_
 check
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -54,16 +54,16 @@
   const auto *PushBackExpr = Result.Nodes.getNodeAs("push_back");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
 
-  auto functionNameSourceRange = CharSourceRange::getCharRange(
+  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   PushBackExpr->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag =
   diag(PushBackExpr->getExprLoc(), "use emplace_back instead of 
push_back");
 
-  if (functionNameSourceRange.getBegin().isMacroID())
+  if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
-  Diag << FixItHint::CreateReplacement(functionNameSourceRange,
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
"emplace_back(");
 
   auto CallParensRange = InnerCtorCall->getParenOrBraceRange();


Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -3,9 +3,8 @@
 modernize-use-emplace
 =
 
-This check would look for cases when inserting new element into an STL
-container, but the element is constructed temporarily or is constructed just
-to be moved. It would also work with std::pair.
+This check look for cases when inserting new element into an STL
+container, but the element is constructed temporarily.
 
 Before:
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -209,7 +209,7 @@
 - New `modernize-use-emplace
   `_ check
 
-  Finds calls that could be change to emplace.
+  Finds calls that could be changed to emplace.
 
 - New `performance-faster-string-find
   `_ check
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -54,16 +54,16 @@
   const auto *PushBackExpr = Result.Nodes.getNodeAs("push_back");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
 
-  auto functionNameSourceRange = CharSourceRange::getCharRange(
+  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   PushBackExpr->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag =
   diag(PushBackExpr->getExprLoc(), "use emplace_back instead of push_back");
 
-  if (functionNameSourceRange.getBegin().isMacroID())
+  if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
-  Diag << FixItHint::CreateReplacement(functionNameSourceRange,
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
"emplace_back(");
 
   auto CallParensRange = InnerCtorCall->getParenOrBraceRange();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 4 inline comments as done.


Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:6
@@ +5,3 @@
+
+This check would look for cases when inserting new element into an STL
+container, but the element is constructed temporarily or is constructed just

sbarzowski wrote:
> Why "would"? Did you copy the description from the proposal? :-)
yep :D


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:28
@@ +27,3 @@
+
+  auto callPushBack =
+  cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),

Shouldn't it start with an uppercase letter?


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:37
@@ +36,3 @@
+  // (and destructed) as in push_back case.
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",

Same


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+
+  auto hasConstructExpr = has(ignoringParenImpCasts(
+  cxxConstructExpr(unless(isCtorOfSmartPtr)).bind("ctor")));

Same


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:44
@@ +43,3 @@
+
+  auto ctorAsArgument = materializeTemporaryExpr(
+  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;

same


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Stanisław Barzowski via cfe-commits]

sbarzowski added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:57
@@ +56,3 @@
+
+  auto functionNameSourceRange = CharSourceRange::getCharRange(
+  PushBackExpr->getExprLoc(), Call->getArg(0)->getExprLoc());

capital letter?


Comment at: docs/ReleaseNotes.rst:212
@@ +211,3 @@
+
+  Finds calls that could be change to emplace.
+

typo: change -> changed


Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:6
@@ +5,3 @@
+
+This check would look for cases when inserting new element into an STL
+container, but the element is constructed temporarily or is constructed just

Why "would"? Did you copy the description from the proposal? :-)


Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:8
@@ +7,3 @@
+container, but the element is constructed temporarily or is constructed just
+to be moved. It would also work with std::pair.
+

Same here, and it is not obvious to me what is so special about std::pair here.


http://reviews.llvm.org/D20964



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 59558.
Prazek added a comment.

- Fix


http://reviews.llvm.org/D20964

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -0,0 +1,157 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
+template 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args);
+};
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair ){};
+  template 
+  pair(pair &){};
+};
+
+template 
+pair make_pair(T1, T2) {
+  return pair();
+};
+
+template 
+class unique_ptr {
+public:
+  unique_ptr(T *) {}
+};
+} // namespace std
+
+void testInts() {
+  std::vector v;
+  v.push_back(42);
+  v.push_back(int(42));
+  v.push_back(int{42});
+  int z;
+  v.push_back(z);
+}
+
+struct S {
+  S(int a, int b = 41) {}
+  S() {}
+  void push_back(S);
+};
+
+struct Convertable {
+  operator S() { return S{}; }
+};
+struct Zoz {
+  Zoz(S s) {}
+};
+
+Zoz getZoz(S s) { return Zoz(s); }
+
+void test_S() {
+  std::vector v;
+
+  v.push_back(S(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(S{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(S());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  v.push_back(S{});
+  // 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);
+
+  S 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 v;
+  v.push_back(Zoz(S(21, 37)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(S(21, 37));
+
+  v.push_back(getZoz(S(1, 2)));
+}
+
+void testPair() {
+  std::vector> v;
+  v.push_back(std::pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  std::vector> v2;
+  v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37)));
+}
+
+void testSpaces() {
+  std::vector v;
+
+  // clang-format off
+  v.push_back(S   (1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+  v.push_back(S   {1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
+  v.push_back(  S {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}}
+  // CHECK-FIXES: v.emplace_back();
+
+  // clang-format on
+}
+
+void testPointers() {
+  std::vector v;
+  v.push_back(new int(5));
+
+  std::vector 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> v;
+  // FIXME: add functionality to change calls of std::make_pair
+  v.push_back(std::make_pair(1, 2));
+}
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - modernize-use-emplace
+
+modernize-use-emplace
+=
+
+This check would look for cases when inserting new element into an STL
+container, but the element is constructed temporarily or is constructed just
+to be moved. It would also work with std::pair.
+
+Before:
+
+.. code:: c++
+
+	std::vector v;
+	v.push_back(MyClass(21, 37));
+
+