[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<std::pair<int,int> > 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<std::unique_ptr<int> > 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<std::unique_ptr<int>> v2;
+  v2.push_back(new int(42));
+  // This call can't be replaced with emplace_back.
----------------
This test is broken. `unique_ptr<T>` 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