Prazek created this revision.
Prazek added reviewers: sbenza, alexfh, aaron.ballman.
Prazek added subscribers: cfe-commits, hokein, sbarzowski, mnbvmar, staronj,
sbenza.
Prazek set the repository for this revision to rL LLVM.
Prazek added a project: clang-extra.
Not everything is valid (as you see in FIXME), but it should works for 99.8%
cases.
I gonna fix it in next week, but I would like to land this bug fixes right now.
This is mainly fixes of the review that @sbenza gave me last time.
Repository:
rL LLVM
http://reviews.llvm.org/D22208
Files:
clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tidy/modernize/UseEmplaceCheck.h
clang-tidy/utils/Matchers.h
docs/clang-tidy/checks/modernize-use-emplace.rst
test/clang-tidy/modernize-use-emplace.cpp
Index: test/clang-tidy/modernize-use-emplace.cpp
===================================================================
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -1,4 +1,7 @@
-// RUN: %check_clang_tidy %s modernize-use-emplace %t
+// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: modernize-use-emplace.containersWithPushBack, \
+// RUN: value: 'std::vector; std::list; std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
namespace std {
template <typename T>
@@ -9,6 +12,7 @@
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~vector();
};
template <typename T>
class list {
@@ -18,6 +22,7 @@
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~list();
};
template <typename T>
@@ -28,6 +33,7 @@
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~deque();
};
template <typename T1, typename T2>
@@ -54,10 +60,24 @@
template <typename T>
class unique_ptr {
public:
- unique_ptr(T *) {}
+ explicit unique_ptr(T *) {}
+ ~unique_ptr();
};
} // namespace std
+namespace llvm {
+template <typename T>
+class LikeASmallVector {
+public:
+ void push_back(const T &) {}
+ void push_back(T &&) {}
+
+ template <typename... Args>
+ void emplace_back(Args &&... args){};
+};
+
+} // llvm
+
void testInts() {
std::vector<int> v;
v.push_back(42);
@@ -117,6 +137,23 @@
v.push_back(s);
}
+template <typename ElemType>
+void dependOnElem() {
+ std::vector<ElemType> v;
+ v.push_back(ElemType(42));
+}
+
+template <typename ContainerType>
+void dependOnContainer() {
+ ContainerType v;
+ v.push_back(Something(42));
+}
+
+void callDependent() {
+ dependOnElem<Something>();
+ dependOnContainer<std::vector<Something>>();
+}
+
void test2() {
std::vector<Zoz> v;
v.push_back(Zoz(Something(21, 37)));
@@ -206,14 +243,14 @@
v.push_back(new int(5));
std::vector<std::unique_ptr<int>> v2;
- v2.push_back(new int(42));
+ v2.push_back(std::unique_ptr<int>(new int(42)));
// This call can't be replaced with emplace_back.
// If emplacement will fail (not enough memory to add to vector)
// we will have leak of int because unique_ptr won't be constructed
// (and destructed) as in push_back case.
auto *ptr = new int;
- v2.push_back(ptr);
+ v2.push_back(std::unique_ptr<int>(ptr));
// Same here
}
@@ -240,6 +277,11 @@
d.push_back(Something(42));
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: d.emplace_back(42);
+
+ llvm::LikeASmallVector<Something> ls;
+ ls.push_back(Something(42));
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+ // CHECK-FIXES: ls.emplace_back(42);
}
class IntWrapper {
@@ -336,3 +378,16 @@
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: v.emplace_back(42, var);
}
+
+struct WithDtor {
+ WithDtor(int) {}
+ ~WithDtor();
+};
+
+void testWithDtor() {
+ std::vector<WithDtor> v;
+
+ v.push_back(WithDtor(42));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+ // CHECK-FIXES: v.emplace_back(42);
+}
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===================================================================
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -4,28 +4,36 @@
=====================
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.
+
+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.
+
Before:
.. code:: c++
std::vector<MyClass> v;
v.push_back(MyClass(21, 37));
- std::vector<std::pair<int,int> > w;
- w.push_back(std::make_pair(21, 37));
+ std::vector<std::pair<int,int>> w;
+
+ w.push_back(std::pair<int,int>(21, 37));
+ w.push_back(std::make_pair(21L, 37L));
After:
.. code:: c++
std::vector<MyClass> v;
v.emplace_back(21, 37);
- std::vector<std::pair<int,int> > w;
- v.emplace_back(21, 37);
+ std::vector<std::pair<int,int>> w;
+ w.emplace_back(21, 37);
+ // This will be fixed to w.push_back(21, 37); in next version
+ w.emplace_back(std::make_pair(21L, 37L);
The other situation is when we pass arguments that will be converted to a type
inside a container.
@@ -45,20 +53,28 @@
v.emplace_back("abc");
+In some cases the transformation would be valid, but the code
+wouldn't be exception safe.
In this case the calls of ``push_back`` won't be replaced.
.. code:: c++
- std::vector<std::unique_ptr<int> > v;
- v.push_back(new int(5));
- auto *ptr = int;
- v.push_back(ptr);
+ std::vector<std::unique_ptr<int>> v;
+ v.push_back(std::unique_ptr<int>(new int(0)));
+ auto *ptr = new int(1);
+ v.push_back(std::unique_ptr<int>(ptr));
This is because replacing it with ``emplace_back`` could cause a leak of this
pointer if ``emplace_back`` would throw exception before emplacement
(e.g. not enough memory to add new element).
For more info read item 42 - "Consider emplacement instead of insertion."
-of Scott Meyers Efective Modern C++.
+of Scott Meyers Effective Modern C++.
+
+The default smart pointers that are considered are
+``std::unique_ptr``, ``std::shared_ptr``, ``std::auto_ptr``.
+To specify other smart pointers or other classes use option
+`smartPointers` similar to `containersWithPushBack`.
+
Check also fires if any argument of constructor call would be:
- bitfield (bitfields can't bind to rvalue/universal reference)
Index: clang-tidy/utils/Matchers.h
===================================================================
--- clang-tidy/utils/Matchers.h
+++ clang-tidy/utils/Matchers.h
@@ -40,8 +40,6 @@
Node, Finder->getASTContext());
}
-AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
-
// Returns QualType matcher for references to const.
AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
using namespace ast_matchers;
Index: clang-tidy/modernize/UseEmplaceCheck.h
===================================================================
--- clang-tidy/modernize/UseEmplaceCheck.h
+++ clang-tidy/modernize/UseEmplaceCheck.h
@@ -11,6 +11,8 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H
#include "../ClangTidy.h"
+#include <string>
+#include <vector>
namespace clang {
namespace tidy {
@@ -20,15 +22,18 @@
/// the element is constructed temporarily.
/// It replaces those calls for emplace_back of arguments passed to
/// constructor of temporary object.
-///`
+///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
class UseEmplaceCheck : public ClangTidyCheck {
public:
- UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ UseEmplaceCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ std::vector<std::string> containersWithPushBack, smartPointers;
};
} // namespace modernize
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===================================================================
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -8,18 +8,48 @@
//===----------------------------------------------------------------------===//
#include "UseEmplaceCheck.h"
-#include "../utils/Matchers.h"
-
+#include "../utils/OptionsUtils.h"
using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
namespace modernize {
+namespace {
+
+llvm::Optional<ast_matchers::internal::Matcher<NamedDecl>>
+getHasAnyName(const std::vector<std::string> &names) {
+ llvm::Optional<ast_matchers::internal::Matcher<NamedDecl>> hasMatcher;
+
+ for (const auto &ClassName : names) {
+ const auto HasName = hasName(ClassName);
+ hasMatcher = hasMatcher ? anyOf(*hasMatcher, HasName) : HasName;
+ }
+ return hasMatcher;
+}
+
+const std::string defaultContainersWithPushBack =
+ "std::vector; std::list; std::deque";
+const std::string defaultSmartPointers =
+ "std::shared_ptr; std::unique_ptr; std::auto_ptr; std::weak_ptr";
+} // namespace
+
+UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ containersWithPushBack(utils::options::parseStringList(Options.get(
+ "containersWithPushBack", defaultContainersWithPushBack))),
+ smartPointers(utils::options::parseStringList(
+ Options.get("smartPointers", defaultSmartPointers))) {}
+
void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
if (!getLangOpts().CPlusPlus11)
return;
+ auto hasContainerName = getHasAnyName(containersWithPushBack);
+ // No containers to match.
+ if (!hasContainerName)
+ return;
+
// FIXME: Bunch of functionality that could be easily added:
// + add handling of `push_front` for std::forward_list, std::list
// and std::deque.
@@ -29,42 +59,48 @@
// regression)
// + match for emplace calls that should be replaced with insertion
// + match for make_pair calls.
- auto callPushBack = cxxMemberCallExpr(
- hasDeclaration(functionDecl(hasName("push_back"))),
- on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
- "std::list", "std::deque")))));
+ auto callPushBack =
+ cxxMemberCallExpr(hasDeclaration(functionDecl(hasName("push_back"))),
+ on(hasType(cxxRecordDecl(*hasContainerName))));
+
+ auto hasSmartPointerName = getHasAnyName(smartPointers);
// We can't replace push_backs of smart pointer because
// if emplacement fails (f.e. bad_alloc in vector) we will have leak of
// passed pointer because smart pointer won't be constructed
// (and destructed) as in push_back case.
- auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
- ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
- "std::weak_ptr"))));
+ auto isCtorOfSmartPtr =
+ hasSmartPointerName
+ ? hasDeclaration(cxxConstructorDecl(ofClass(*hasSmartPointerName)))
+ : hasDeclaration(unless(anything()));
// Bitfields binds only to consts and emplace_back take it by universal ref.
- auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(
- memberExpr(hasDeclaration(fieldDecl(matchers::isBitfield())))));
+ auto bitFieldAsArgument = hasAnyArgument(
+ ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField())))));
// We could have leak of resource.
- auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr()));
+ auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
auto constructingDerived =
hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
- auto hasInitList = has(ignoringParenImpCasts(initListExpr()));
+ auto hasInitList = has(ignoringImplicit(initListExpr()));
+ // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
+ // overloaded functions and template names.
auto soughtConstructExpr =
cxxConstructExpr(
unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
newExprAsArgument, constructingDerived,
has(materializeTemporaryExpr(hasInitList)))))
.bind("ctor");
- auto hasConstructExpr = has(ignoringParenImpCasts(soughtConstructExpr));
+ auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
auto ctorAsArgument = materializeTemporaryExpr(
anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr))));
- Finder->addMatcher(
- cxxMemberCallExpr(callPushBack, has(ctorAsArgument)).bind("call"), this);
+ Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+ unless(isInTemplateInstantiation()))
+ .bind("call"),
+ this);
}
void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
@@ -89,14 +125,19 @@
return;
// Range for constructor name and opening brace.
- auto CtorCallSourceRange = CharSourceRange::getCharRange(
- InnerCtorCall->getExprLoc(),
- CallParensRange.getBegin().getLocWithOffset(1));
+ auto CtorCallSourceRange = CharSourceRange::getTokenRange(
+ InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
Diag << FixItHint::CreateRemoval(CtorCallSourceRange)
- << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
- CallParensRange.getEnd(),
- CallParensRange.getEnd().getLocWithOffset(1)));
+ << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+ CallParensRange.getEnd(), CallParensRange.getEnd()));
+}
+
+void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "containersWithPushBack",
+ utils::options::serializeStringList(containersWithPushBack));
+ Options.store(Opts, "smartPointers",
+ utils::options::serializeStringList(smartPointers));
}
} // namespace modernize
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
