[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<UseNullptrCheck>("modernize-use-nullptr");
+    CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseOverrideCheck>("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<vector<int>> 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