[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts created this revision.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.
joeywatts updated this revision to Diff 451689.
joeywatts added a comment.
Eugene.Zelenko retitled this revision from "(wip) improve modernize-use-emplace 
check" to "[clang-tidy][wip] improve modernize-use-emplace check".
Eugene.Zelenko added reviewers: alexfh, aaron.ballman, njames93, 
LegalizeAdulthood, JonasToth.
Eugene.Zelenko added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
joeywatts updated this revision to Diff 451842.
joeywatts retitled this revision from "[clang-tidy][wip] improve 
modernize-use-emplace check" to "[clang-tidy] Improve modernize-use-emplace 
check".
joeywatts edited the summary of this revision.
joeywatts updated this revision to Diff 451847.
joeywatts published this revision for review.
Herald added a subscriber: cfe-commits.

Update documentation


Eugene.Zelenko added a comment.

Please mention changes in Release Notes.


joeywatts added a comment.

Updated commit title/description.


joeywatts added a comment.

Add changes to release notes.


This patch improves the modernize-use-emplace check by adding support for
detecting inefficient invocations of the `push` and `push_front` methods on
STL-style containers and replacing them with their `emplace`-style equivalent.

Fixes #56996.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131623

Files:
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
@@ -62,6 +62,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -86,6 +89,9 @@
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   iterator emplace(const_iterator pos, Args &&...args){};
   template 
@@ -104,6 +110,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   void emplace_front(Args &&...args){};
   template 
@@ -235,6 +244,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -244,6 +256,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -253,6 +268,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -667,15 +685,43 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: l.emplace_back(42, 41);
 
+  l.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: l.emplace_front(42, 41);
+
   std::deque d;
   d.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: d.emplace_back(42);
 
+  d.push_front(Something(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: d.emplace_front(42);
+
   llvm::LikeASmallVector ls;
   ls.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
   // CHECK-FIXES: ls.emplace_back(42);
+
+  std::stack s;
+  s.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: s.emplace(42, 41);
+
+  std::queue q;
+  q.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: q.emplace(42, 41);
+
+  std::priority_queue pq;
+  pq.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace
+  // CHECK-FIXES: pq.emplace(42, 41);
+
+  std::forward_list fl;
+  fl.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_front
+  // CHECK-FIXES: fl.emplace_front(42, 41);
 }
 
 class IntWrapper {
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
@@ -4,16 +4,22 @@
 =
 
 The check flags insertions to an STL-style container done by calling the
-``push_back

[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts updated this revision to Diff 451850.
joeywatts added a comment.

Update commit title/message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131623/new/

https://reviews.llvm.org/D131623

Files:
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
@@ -62,6 +62,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -86,6 +89,9 @@
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   iterator emplace(const_iterator pos, Args &&...args){};
   template 
@@ -104,6 +110,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   void emplace_front(Args &&...args){};
   template 
@@ -235,6 +244,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -244,6 +256,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -253,6 +268,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -667,15 +685,43 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: l.emplace_back(42, 41);
 
+  l.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: l.emplace_front(42, 41);
+
   std::deque d;
   d.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: d.emplace_back(42);
 
+  d.push_front(Something(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: d.emplace_front(42);
+
   llvm::LikeASmallVector ls;
   ls.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
   // CHECK-FIXES: ls.emplace_back(42);
+
+  std::stack s;
+  s.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: s.emplace(42, 41);
+
+  std::queue q;
+  q.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: q.emplace(42, 41);
+
+  std::priority_queue pq;
+  pq.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace
+  // CHECK-FIXES: pq.emplace(42, 41);
+
+  std::forward_list fl;
+  fl.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_front
+  // CHECK-FIXES: fl.emplace_front(42, 41);
 }
 
 class IntWrapper {
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
@@ -4,16 +4,22 @@
 =
 
 The check flags insertions to an STL-style container done by calling the
-``push_back`` method with an explicitly-constructed temporary of the container
-element type. In this case, the corresponding ``emplace_back`` method
-results in less verbose and potentially more efficient code.
-Right now the check doesn't support ``push_front`` and ``insert``.
-It also doesn't support ``insert`` functions for associative containers
-because replacing ``insert`` with ``emplace`` may result in
+``push_back``, ``push``, or ``push_front`` methods with an
+explicitly-constructed temporary of the container element type. In this case,
+the corresponding ``emplace`` equivalent methods result in less verbose and
+potentially more efficient code.  Right now the check doesn't support
+``insert``. It also doesn't support ``insert`` functions for associative
+containers because replacing ``insert`` with ``emplace`` may result in
 `speed regression `_, but it might get support with some addition flag in the future.
 
-By default only ``std::vector``, ``std::deque``, ``std::list`` are considered.
-This list can be modified using the :option:`ContainersWithPushBack` option.

[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts added a comment.

In D131623#3716227 , @njames93 wrote:

> Just a general drive by comment here and doesn't affect this patch. 
> This specifying containers logic is a little verbose, There may be a case to 
> deprecate most of these options and just detect containers with an equivalent 
> emplace* method at runtime.

I thought the same thing, but wasn't sure what the stance is on breaking 
changes here: would it be a problem if the existing "ContainersWithPush" option 
were removed in favor of a more generic option for a list of containers?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131623/new/

https://reviews.llvm.org/D131623

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

[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts updated this revision to Diff 451960.
joeywatts added a comment.

Address code review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131623/new/

https://reviews.llvm.org/D131623

Files:
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
@@ -62,6 +62,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -86,6 +89,9 @@
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   iterator emplace(const_iterator pos, Args &&...args){};
   template 
@@ -104,6 +110,9 @@
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template 
   void emplace_front(Args &&...args){};
   template 
@@ -235,6 +244,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -244,6 +256,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -253,6 +268,9 @@
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template 
   void emplace(Args &&...args){};
 };
@@ -667,15 +685,43 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: l.emplace_back(42, 41);
 
+  l.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: l.emplace_front(42, 41);
+
   std::deque d;
   d.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: d.emplace_back(42);
 
+  d.push_front(Something(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+  // CHECK-FIXES: d.emplace_front(42);
+
   llvm::LikeASmallVector ls;
   ls.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
   // CHECK-FIXES: ls.emplace_back(42);
+
+  std::stack s;
+  s.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: s.emplace(42, 41);
+
+  std::queue q;
+  q.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: q.emplace(42, 41);
+
+  std::priority_queue pq;
+  pq.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace
+  // CHECK-FIXES: pq.emplace(42, 41);
+
+  std::forward_list fl;
+  fl.push_front(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_front
+  // CHECK-FIXES: fl.emplace_front(42, 41);
 }
 
 class IntWrapper {
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
@@ -4,16 +4,22 @@
 =
 
 The check flags insertions to an STL-style container done by calling the
-``push_back`` method with an explicitly-constructed temporary of the container
-element type. In this case, the corresponding ``emplace_back`` method
-results in less verbose and potentially more efficient code.
-Right now the check doesn't support ``push_front`` and ``insert``.
-It also doesn't support ``insert`` functions for associative containers
-because replacing ``insert`` with ``emplace`` may result in
+``push_back``, ``push``, or ``push_front`` methods with an
+explicitly-constructed temporary of the container element type. In this case,
+the corresponding ``emplace`` equivalent methods result in less verbose and
+potentially more efficient code.  Right now the check doesn't support
+``insert``. It also doesn't support ``insert`` functions for associative
+containers because replacing ``insert`` with ``emplace`` may result in
 `speed regression `_, but it might get support with some addition flag in the future.
 
-By default only ``std::vector``, ``std::deque``, ``std::list`` are considered.
-This list can be modified using the :option:`ContainersWithPushBack` option

[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts added a comment.

Thanks for the review @njames93! This is my first contribution so I don't think 
I have permission to land this myself, is there someone that can do that for me?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131623/new/

https://reviews.llvm.org/D131623

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

[PATCH] D131623: [clang-tidy] Improve modernize-use-emplace check

[Joey Watts via Phabricator via cfe-commits]

joeywatts added a comment.

In D131623#3731106 , @njames93 wrote:

> In D131623#3730833 , @joeywatts 
> wrote:
>
>> Thanks for the review @njames93! This is my first contribution so I don't 
>> think I have permission to land this myself, is there someone that can do 
>> that for me?
>
> Sure, which email would you like me to use for the commit?

Please use jwatt...@bloomberg.net!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131623/new/

https://reviews.llvm.org/D131623

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