forked from OSchip/llvm-project
[clang-tidy] Fixes to modernize-use-emplace
Not everything is valid, but it should works for 99.8% cases https://reviews.llvm.org/D22208 llvm-svn: 277097
This commit is contained in:
Piotr Padlewski
parent
0a9cbd4743
commit
8f80229109
|
|
@ -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(MatchFinder *Finder) {
|
|||
// + 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 MatchFinder::MatchResult &Result) {
|
|||
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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -40,8 +40,6 @@ AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) {
|
|||
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;
|
||||
|
|
|
|||
|
|
@ -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 constructor call would be:
|
|||
or if the argument would be converted via derived-to-base cast.
|
||||
|
||||
This check requires C++11 of higher to run.
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue