[clang-tools-extra] b8655f7 - [clang-tidy] Improve modernize-use-emplace check
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 18 23:57:58 PDT 2022
Author: Joey Watts
Date: 2022-08-19T07:57:42+01:00
New Revision: b8655f7ff286b9ebcd97cdd24b9c8eb5b89b9651
URL: https://github.com/llvm/llvm-project/commit/b8655f7ff286b9ebcd97cdd24b9c8eb5b89b9651
DIFF: https://github.com/llvm/llvm-project/commit/b8655f7ff286b9ebcd97cdd24b9c8eb5b89b9651.diff
LOG: [clang-tidy] Improve modernize-use-emplace check
This patch improves the modernize-use-emplace check by adding support for
detecting inefficient invocations of the `push` and `push_front` methods on
STL-style containers and replacing them with their `emplace`-style equivalent.
Fixes #56996.
Reviewed By: njames93
Differential Revision: https://reviews.llvm.org/D131623
Added:
Modified:
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
index e6fb4c0a87264..abf5ba918d89f 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -84,6 +84,10 @@ AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
const auto DefaultContainersWithPushBack =
"::std::vector; ::std::list; ::std::deque";
+const auto DefaultContainersWithPush =
+ "::std::stack; ::std::queue; ::std::priority_queue";
+const auto DefaultContainersWithPushFront =
+ "::std::forward_list; ::std::list; ::std::deque";
const auto DefaultSmartPointers =
"::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
const auto DefaultTupleTypes = "::std::pair; ::std::tuple";
@@ -109,6 +113,10 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
"IgnoreImplicitConstructors", false)),
ContainersWithPushBack(utils::options::parseStringList(Options.get(
"ContainersWithPushBack", DefaultContainersWithPushBack))),
+ ContainersWithPush(utils::options::parseStringList(
+ Options.get("ContainersWithPush", DefaultContainersWithPush))),
+ ContainersWithPushFront(utils::options::parseStringList(Options.get(
+ "ContainersWithPushFront", DefaultContainersWithPushFront))),
SmartPointers(utils::options::parseStringList(
Options.get("SmartPointers", DefaultSmartPointers))),
TupleTypes(utils::options::parseStringList(
@@ -120,9 +128,6 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
// FIXME: Bunch of functionality that could be easily added:
- // + add handling of `push_front` for std::forward_list, std::list
- // and std::deque.
- // + add handling of `push` for std::stack, std::queue, std::priority_queue
// + add handling of `insert` for stl associative container, but be careful
// because this requires special treatment (it could cause performance
// regression)
@@ -131,6 +136,14 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
hasDeclaration(functionDecl(hasName("push_back"))),
on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPushBack)))));
+ auto CallPush = cxxMemberCallExpr(
+ hasDeclaration(functionDecl(hasName("push"))),
+ on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPush)))));
+
+ auto CallPushFront = cxxMemberCallExpr(
+ hasDeclaration(functionDecl(hasName("push_front"))),
+ on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPushFront)))));
+
auto CallEmplacy = cxxMemberCallExpr(
hasDeclaration(
functionDecl(hasAnyNameIgnoringTemplates(EmplacyFunctions))),
@@ -208,6 +221,18 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
.bind("push_back_call")),
this);
+ Finder->addMatcher(
+ traverse(TK_AsIs, cxxMemberCallExpr(CallPush, has(SoughtParam),
+ unless(isInTemplateInstantiation()))
+ .bind("push_call")),
+ this);
+
+ Finder->addMatcher(
+ traverse(TK_AsIs, cxxMemberCallExpr(CallPushFront, has(SoughtParam),
+ unless(isInTemplateInstantiation()))
+ .bind("push_front_call")),
+ this);
+
Finder->addMatcher(
traverse(TK_AsIs,
cxxMemberCallExpr(
@@ -237,15 +262,29 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
const auto *PushBackCall =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
+ const auto *PushCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_call");
+ const auto *PushFrontCall =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call");
const auto *EmplacyCall =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make");
- assert((PushBackCall || EmplacyCall) && "No call matched");
- assert((CtorCall || MakeCall) && "No push_back parameter matched");
+ const CXXMemberCallExpr *Call = [&]() {
+ if (PushBackCall) {
+ return PushBackCall;
+ }
+ if (PushCall) {
+ return PushCall;
+ }
+ if (PushFrontCall) {
+ return PushFrontCall;
+ }
+ return EmplacyCall;
+ }();
- const CXXMemberCallExpr *Call = PushBackCall ? PushBackCall : EmplacyCall;
+ assert(Call && "No call matched");
+ assert((CtorCall || MakeCall) && "No push_back parameter matched");
if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 &&
CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange())
@@ -255,11 +294,19 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
Call->getExprLoc(), Call->getArg(0)->getExprLoc());
auto Diag =
- PushBackCall
- ? diag(Call->getExprLoc(), "use emplace_back instead of push_back")
- : diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(),
- "unnecessary temporary object created while calling " +
- Call->getMethodDecl()->getName().str());
+ EmplacyCall
+ ? diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(),
+ "unnecessary temporary object created while calling %0")
+ : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 "
+ "instead of push%select{|_back|_front}0");
+ if (EmplacyCall)
+ Diag << Call->getMethodDecl()->getName();
+ else if (PushCall)
+ Diag << 0;
+ else if (PushBackCall)
+ Diag << 1;
+ else
+ Diag << 2;
if (FunctionNameSourceRange.getBegin().isMacroID())
return;
@@ -268,6 +315,14 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
const char *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back(";
Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
EmplacePrefix);
+ } else if (PushCall) {
+ const char *EmplacePrefix = MakeCall ? "emplace" : "emplace(";
+ Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
+ EmplacePrefix);
+ } else if (PushFrontCall) {
+ const char *EmplacePrefix = MakeCall ? "emplace_front" : "emplace_front(";
+ Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
+ EmplacePrefix);
}
const SourceRange CallParensRange =
@@ -302,6 +357,10 @@ void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IgnoreImplicitConstructors", IgnoreImplicitConstructors);
Options.store(Opts, "ContainersWithPushBack",
utils::options::serializeStringList(ContainersWithPushBack));
+ Options.store(Opts, "ContainersWithPush",
+ utils::options::serializeStringList(ContainersWithPush));
+ Options.store(Opts, "ContainersWithPushFront",
+ utils::options::serializeStringList(ContainersWithPushFront));
Options.store(Opts, "SmartPointers",
utils::options::serializeStringList(SmartPointers));
Options.store(Opts, "TupleTypes",
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
index 779abf6521ada..4fe788e5d61e4 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
@@ -37,6 +37,8 @@ class UseEmplaceCheck : public ClangTidyCheck {
private:
const bool IgnoreImplicitConstructors;
const std::vector<StringRef> ContainersWithPushBack;
+ const std::vector<StringRef> ContainersWithPush;
+ const std::vector<StringRef> ContainersWithPushFront;
const std::vector<StringRef> SmartPointers;
const std::vector<StringRef> TupleTypes;
const std::vector<StringRef> TupleMakeFunctions;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fe3338385a638..9e5e176336468 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -120,6 +120,12 @@ Changes in existing checks
<clang-tidy/checks/bugprone/signal-handler>` check. Partial
support for C++14 signal handler rules was added. Bug report generation was
improved.
+
+- Improved `modernize-use-emplace <clang-tidy/checks/modernize/use-emplace.html>`_ check.
+
+ The check now supports detecting inefficient invocations of ``push`` and
+ ``push_front`` on STL-style containers and replacing them with ``emplace``
+ or ``emplace_front``.
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
index f08bfb4449c32..f61b93aac7c76 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst
@@ -4,16 +4,22 @@ modernize-use-emplace
=====================
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
+``push_back``, ``push``, or ``push_front`` methods with an
+explicitly-constructed temporary of the container element type. In this case,
+the corresponding ``emplace`` equivalent methods result in less verbose and
+potentially more efficient code. Right now the check doesn't support
+``insert``. It also doesn't support ``insert`` functions for associative
+containers because replacing ``insert`` with ``emplace`` may result in
`speed regression <https://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 using the :option:`ContainersWithPushBack` option.
+The :option:`ContainersWithPushBack`, :option:`ContainersWithPush`, and
+:option:`ContainersWithPushFront` options are used to specify the container
+types that support the ``push_back``, ``push``, and ``push_front`` operations
+respectively. The default values for these options are as follows:
+
+* :option:`ContainersWithPushBack`: ``std::vector``, ``std::deque``, and ``std::list``.
+* :option:`ContainersWithPush`: ``std::stack``, ``std::queue``, and ``std::priority_queue``.
+* :option:`ContainersWithPushFront`: ``std::forward_list``, ``std::list``, and ``std::deque``.
This check also reports when an ``emplace``-like method is improperly used,
for example using ``emplace_back`` while also calling a constructor. This
@@ -116,6 +122,16 @@ Options
Semicolon-separated list of class names of custom containers that support
``push_back``.
+.. option:: ContainersWithPush
+
+ Semicolon-separated list of class names of custom containers that support
+ ``push``.
+
+.. option:: ContainersWithPushFront
+
+ Semicolon-separated list of class names of custom containers that support
+ ``push_front``.
+
.. option:: IgnoreImplicitConstructors
When `true`, the check will ignore implicitly constructed arguments of
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
index b869816f8d1da..29ba88117b5f7 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
@@ -62,6 +62,9 @@ class list {
class const_iterator {};
const_iterator begin() { return const_iterator{}; }
+ void push_front(const T &) {}
+ void push_front(T &&) {}
+
void push_back(const T &) {}
void push_back(T &&) {}
@@ -86,6 +89,9 @@ class deque {
void push_back(const T &) {}
void push_back(T &&) {}
+ void push_front(const T &) {}
+ void push_front(T &&) {}
+
template <typename... Args>
iterator emplace(const_iterator pos, Args &&...args){};
template <typename... Args>
@@ -104,6 +110,9 @@ class forward_list {
class const_iterator {};
const_iterator begin() { return const_iterator{}; }
+ void push_front(const T &) {}
+ void push_front(T &&) {}
+
template <typename... Args>
void emplace_front(Args &&...args){};
template <typename... Args>
@@ -235,6 +244,9 @@ class stack {
public:
using value_type = T;
+ void push(const T &) {}
+ void push(T &&) {}
+
template <typename... Args>
void emplace(Args &&...args){};
};
@@ -244,6 +256,9 @@ class queue {
public:
using value_type = T;
+ void push(const T &) {}
+ void push(T &&) {}
+
template <typename... Args>
void emplace(Args &&...args){};
};
@@ -253,6 +268,9 @@ class priority_queue {
public:
using value_type = T;
+ void push(const T &) {}
+ void push(T &&) {}
+
template <typename... Args>
void emplace(Args &&...args){};
};
@@ -667,15 +685,43 @@ void testOtherContainers() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: l.emplace_back(42, 41);
+ l.push_front(Something(42, 41));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+ // CHECK-FIXES: l.emplace_front(42, 41);
+
std::deque<Something> d;
d.push_back(Something(42));
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
// CHECK-FIXES: d.emplace_back(42);
+ d.push_front(Something(42));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front
+ // CHECK-FIXES: d.emplace_front(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);
+
+ std::stack<Something> s;
+ s.push(Something(42, 41));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+ // CHECK-FIXES: s.emplace(42, 41);
+
+ std::queue<Something> q;
+ q.push(Something(42, 41));
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+ // CHECK-FIXES: q.emplace(42, 41);
+
+ std::priority_queue<Something> pq;
+ pq.push(Something(42, 41));
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace
+ // CHECK-FIXES: pq.emplace(42, 41);
+
+ std::forward_list<Something> fl;
+ fl.push_front(Something(42, 41));
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_front
+ // CHECK-FIXES: fl.emplace_front(42, 41);
}
class IntWrapper {
More information about the cfe-commits
mailing list