[PATCH] D22208: clang-tidy] Fixes to modernize-use-emplace

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:31
@@ +30,3 @@
+
+const std::string DefaultContainersWithPushBack =
+    "::std::vector; ::std::list; ::std::deque";
----------------
A std::string is not needed here, please change to `const char []`.

================
Comment at: clang-tidy/modernize/UseEmplaceCheck.h:36
@@ -32,1 +35,3 @@
+private:
+  std::vector<std::string> ContainersWithPushBack, SmartPointers;
 };
----------------
Declarations with multiple variables can be misleading. Please split this into two separate declarations.

================
Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:7
@@ -6,3 +6,3 @@
 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.
+container, but the element is constructed temporarily.
+
----------------
IMO, this sentence fails to convey the most important parts of the information. Consider changing to something like this:
> 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.

Maybe also explain, why the check only looks for `push_back`, and not `insert`, `push_front` and `push`.

================
Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:10
@@ +9,3 @@
+By default only ``std::vector``, ``std::deque``, ``std::list`` are considered.
+This list can be modified by passing a `;` separated list of class names using the `ContainersWithPushBack`
+option.
----------------
s/`;` separated/semicolon-separated/


Repository:
  rL LLVM

https://reviews.llvm.org/D22208