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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
