[clang-tools-extra] r277097 - [clang-tidy] Fixes to modernize-use-emplace
Piotr Padlewski via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 28 19:10:24 PDT 2016
Author: prazek
Date: Thu Jul 28 21:10:23 2016
New Revision: 277097
URL: http://llvm.org/viewvc/llvm-project?rev=277097&view=rev
Log:
[clang-tidy] Fixes to modernize-use-emplace
Not everything is valid, but it should works for 99.8% cases
https://reviews.llvm.org/D22208
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp?rev=277097&r1=277096&r2=277097&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp Thu Jul 28 21:10:23 2016
@@ -8,14 +8,25 @@
//===----------------------------------------------------------------------===//
#include "UseEmplaceCheck.h"
-#include "../utils/Matchers.h"
-
+#include "../utils/OptionsUtils.h"
using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
namespace modernize {
+static const auto DefaultContainersWithPushBack =
+ "::std::vector; ::std::list; ::std::deque";
+static const auto DefaultSmartPointers =
+ "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+
+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;
@@ -31,40 +42,51 @@ void UseEmplaceCheck::registerMatchers(M
// + 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")))));
+ on(hasType(cxxRecordDecl(hasAnyName(SmallVector<StringRef, 5>(
+ ContainersWithPushBack.begin(), ContainersWithPushBack.end()))))));
// 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 = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+ SmallVector<StringRef, 5>(SmartPointers.begin(), SmartPointers.end())))));
// 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())))));
+
+ // Initializer list can't be passed to universal reference.
+ auto initializerListAsArgument = hasAnyArgument(
+ ignoringImplicit(cxxConstructExpr(isListInitialization())));
// We could have leak of resource.
- auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr()));
+ auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+ // We would call another constructor.
auto constructingDerived =
hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
- auto hasInitList = has(ignoringParenImpCasts(initListExpr()));
+ // emplace_back can't access private constructor.
+ auto isPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
+
+ 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)))))
+ initializerListAsArgument, newExprAsArgument,
+ constructingDerived, isPrivateCtor)))
.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 +111,19 @@ void UseEmplaceCheck::check(const MatchF
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
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h?rev=277097&r1=277096&r2=277097&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h Thu Jul 28 21:10:23 2016
@@ -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,19 @@ namespace modernize {
/// 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;
+ std::vector<std::string> SmartPointers;
};
} // namespace modernize
Modified: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/Matchers.h?rev=277097&r1=277096&r2=277097&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h Thu Jul 28 21:10:23 2016
@@ -40,8 +40,6 @@ AST_MATCHER(RecordDecl, isTriviallyDefau
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;
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst?rev=277097&r1=277096&r2=277097&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst Thu Jul 28 21:10:23 2016
@@ -3,9 +3,18 @@
modernize-use-emplace
=====================
-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.
+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.
+Right now the check doesn't support ``push_front`` and ``insert``.
+It also doesn't support ``insert`` functions for associative containers
+because replacing ``insert`` with ``emplace`` may result in
+`speed regression <http://htmlpreview.github.io/?https://github.com/HowardHinnant/papers/blob/master/insert_vs_emplace.html>`_, but it might get support with some addition flag in the future.
+
+By default only ``std::vector``, ``std::deque``, ``std::list`` are considered.
+This list can be modified by passing a semicolon-separated list of class names
+using the `ContainersWithPushBack` option.
Before:
@@ -14,8 +23,10 @@ Before:
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:
@@ -24,8 +35,10 @@ After:
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,21 +58,29 @@ After:
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)
@@ -67,4 +88,3 @@ Check also fires if any argument of cons
or if the argument would be converted via derived-to-base cast.
This check requires C++11 of higher to run.
-
Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp?rev=277097&r1=277096&r2=277097&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp Thu Jul 28 21:10:23 2016
@@ -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 @@ public:
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~vector();
};
template <typename T>
class list {
@@ -18,6 +22,7 @@ public:
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~list();
};
template <typename T>
@@ -28,6 +33,7 @@ public:
template <typename... Args>
void emplace_back(Args &&... args){};
+ ~deque();
};
template <typename T1, typename T2>
@@ -54,10 +60,24 @@ pair<T1, T2> make_pair(T1, T2) {
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);
@@ -72,6 +92,7 @@ struct Something {
Something(int a, int b = 41) {}
Something() {}
void push_back(Something);
+ int getInt() { return 42; }
};
struct Convertable {
@@ -103,6 +124,15 @@ void test_Something() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: v.emplace_back();
+ Something Different;
+ v.push_back(Something(Different.getInt(), 42));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+ // CHECK-FIXES: v.emplace_back(Different.getInt(), 42);
+
+ v.push_back(Different.getInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+ // CHECK-FIXES: v.emplace_back(Different.getInt());
+
v.push_back(42);
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: v.emplace_back(42);
@@ -117,6 +147,23 @@ void test_Something() {
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)));
@@ -130,12 +177,20 @@ void test2() {
v.push_back(getZoz(Something(1, 2)));
}
+struct GetPair {
+ std::pair<int, long> getPair();
+};
void testPair() {
std::vector<std::pair<int, int>> v;
v.push_back(std::pair<int, int>(1, 2));
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: v.emplace_back(1, 2);
+ GetPair g;
+ v.push_back(g.getPair());
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+ // CHECK-FIXES: v.emplace_back(g.getPair());
+
std::vector<std::pair<Something, Zoz>> v2;
v2.push_back(std::pair<Something, Zoz>(Something(42, 42), Zoz(Something(21, 37))));
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
@@ -206,14 +261,14 @@ void testPointers() {
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 +295,11 @@ void testOtherCointainers() {
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 +396,29 @@ void testBitfields() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: v.emplace_back(42, var);
}
+
+class PrivateCtor {
+ PrivateCtor(int z);
+
+public:
+ void doStuff() {
+ std::vector<PrivateCtor> v;
+ // This should not change it because emplace back doesn't have permission.
+ // Check currently doesn't support friend delcarations because pretty much
+ // nobody would want to be friend with std::vector :(.
+ v.push_back(PrivateCtor(42));
+ }
+};
+
+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);
+}
More information about the cfe-commits
mailing list