Index: clang-tidy/modernize/CMakeLists.txt =================================================================== --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeLists.txt @@ -17,6 +17,7 @@ UseAutoCheck.cpp UseBoolLiteralsCheck.cpp UseDefaultCheck.cpp + UseEmplaceCheck.cpp UseNullptrCheck.cpp UseOverrideCheck.cpp Index: clang-tidy/modernize/ModernizeTidyModule.cpp =================================================================== --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -23,6 +23,7 @@ #include "UseAutoCheck.h" #include "UseBoolLiteralsCheck.h" #include "UseDefaultCheck.h" +#include "UseEmplaceCheck.h" #include "UseNullptrCheck.h" #include "UseOverrideCheck.h" @@ -55,6 +56,7 @@ "modernize-use-bool-literals"); CheckFactories.registerCheck("modernize-use-default"); CheckFactories.registerCheck("modernize-use-nullptr"); + CheckFactories.registerCheck("modernize-use-emplace"); CheckFactories.registerCheck("modernize-use-override"); } Index: clang-tidy/modernize/UseEmplaceCheck.h =================================================================== --- /dev/null +++ clang-tidy/modernize/UseEmplaceCheck.h @@ -0,0 +1,38 @@ +//===--- UseEmplaceCheck.h - clang-tidy--------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// This check look for cases when inserting new element into std::vector but +/// 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) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H Index: clang-tidy/modernize/UseEmplaceCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/modernize/UseEmplaceCheck.cpp @@ -0,0 +1,92 @@ +//===--- UseEmplaceCheck.cpp - clang-tidy----------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "UseEmplaceCheck.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + // 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 cerfull + // because this requires special treatment (it could cause performance + // 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", "std::list", "std::deque"))))) + .bind("call"); + + // We can't replace push_backs of smart pointer becase + // if emplacement will fail (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 hasConstructExpr = has(ignoringParenImpCasts( + cxxConstructExpr(unless(isCtorOfSmartPtr)).bind("ctor"))); + + auto ctorAsArgument = materializeTemporaryExpr( + anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr)))); + + Finder->addMatcher(stmt(allOf(callPushBack, has(ctorAsArgument), + has(memberExpr().bind("push_back")))), + this); +} + +void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs("call"); + const auto *PushBackExpr = Result.Nodes.getNodeAs("push_back"); + const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor"); + + auto FunctionNameSourceRange = CharSourceRange::getCharRange( + PushBackExpr->getExprLoc(), Call->getArg(0)->getExprLoc()); + + auto Diag = + diag(PushBackExpr->getExprLoc(), "use emplace_back instead of push_back"); + + if (FunctionNameSourceRange.getBegin().isMacroID()) + return; + + Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, + "emplace_back("); + + auto CallParensRange = InnerCtorCall->getParenOrBraceRange(); + + // Finish if there is no explicit constructor call. + if (CallParensRange.getBegin().isInvalid()) + return; + + // Range for constructor name and opening brace + auto ctorCallSourceRange = CharSourceRange::getCharRange( + InnerCtorCall->getExprLoc(), + CallParensRange.getBegin().getLocWithOffset(1)); + auto ClosingBrace = Call->getLocEnd(); + + Diag << FixItHint::CreateRemoval(ctorCallSourceRange) + << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + CallParensRange.getEnd(), ClosingBrace)); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -206,6 +206,11 @@ Finds integer literals which are cast to ``bool``. +- New `modernize-use-emplace + `_ check + + Finds calls that could be changed to emplace. + - New `performance-faster-string-find `_ check Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -104,6 +104,7 @@ modernize-use-auto modernize-use-bool-literals modernize-use-default + modernize-use-emplace modernize-use-nullptr modernize-use-override performance-faster-string-find Index: docs/clang-tidy/checks/modernize-use-emplace.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/modernize-use-emplace.rst @@ -0,0 +1,60 @@ +.. title:: clang-tidy - modernize-use-emplace + +modernize-use-emplace +===================== + +This check look for cases when inserting new element into an STL +container, but the element is constructed temporarily. + +Before: + +.. code:: c++ + + std::vector v; + v.push_back(MyClass(21, 37)); + + std::vector > w; + w.push_back(std::make_pair(21, 37)); + +After: + +.. code:: c++ + + std::vector v; + v.emplace_back(21, 37); + + std::vector > w; + v.emplace_back(21, 37); + +The other situation is when we pass arguments that will be converted to a type +inside a container. + +Before: + +.. code:: c++ + + std::vector > v; + v.push_back("abc"); + +After: + +.. code:: c++ + + std::vector > v; + v.emplace_back("abc"); + + +In this case the calls of push_back won't be replaced. + +.. code:: c++ + std::vector > v; + v.push_back(new int(5)); + auto *ptr = int; + v.push_back(ptr); + +This is because replacing it with emplace_back could cause a leak of this +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++. Index: test/clang-tidy/modernize-use-emplace.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-emplace.cpp @@ -0,0 +1,189 @@ +// RUN: %check_clang_tidy %s modernize-use-emplace %t + +namespace std { +template +class vector { +public: + void push_back(const T &) {} + void push_back(T &&) {} + + template + void emplace_back(Args &&... args); +}; +template +class list { +public: + void push_back(const T &) {} + void push_back(T &&) {} + + template + void emplace_back(Args &&... args); +}; + +template +class deque { +public: + void push_back(const T &) {} + void push_back(T &&) {} + + template + void emplace_back(Args &&... args); +}; + +template +class pair { +public: + pair() = default; + pair(const pair &) = default; + pair(pair &&) = default; + + pair(const T1 &, const T2 &) {} + pair(T1 &&, T2 &&) {} + + template + pair(const pair &p){}; + template + pair(pair &&p){}; +}; + +template +pair make_pair(T1, T2) { + return pair(); +}; + +template +class unique_ptr { +public: + unique_ptr(T *) {} +}; +} // namespace std + +void testInts() { + std::vector v; + v.push_back(42); + v.push_back(int(42)); + v.push_back(int{42}); + int z; + v.push_back(z); +} + +struct S { + S(int a, int b = 41) {} + S() {} + void push_back(S); +}; + +struct Convertable { + operator S() { return S{}; } +}; +struct Zoz { + Zoz(S s) {} +}; + +Zoz getZoz(S s) { return Zoz(s); } + +void test_S() { + std::vector v; + + v.push_back(S(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back instead of push_back [modernize-use-emplace] + // CHECK-FIXES: v.emplace_back(1, 2); + + v.push_back(S{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(1, 2); + + v.push_back(S()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(); + + v.push_back(S{}); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(); + + v.push_back(42); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(42); + + S temporary(42, 42); + temporary.push_back(temporary); + v.push_back(temporary); + + v.push_back(Convertable()); + v.push_back(Convertable{}); + Convertable s; + v.push_back(s); +} + +void test2() { + std::vector v; + v.push_back(Zoz(S(21, 37))); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(S(21, 37)); + + v.push_back(getZoz(S(1, 2))); +} + +void testPair() { + std::vector> v; + v.push_back(std::pair(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(1, 2); + + std::vector> v2; + v2.push_back(std::pair(S(42, 42), Zoz(S(21, 37)))); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back {{..}} + // CHECK-FIXES: v2.emplace_back(S(42, 42), Zoz(S(21, 37))); +} + +void testSpaces() { + std::vector v; + + // clang-format off + v.push_back( S (1, 2) ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(1, 2); + v.push_back( S {1, 2} ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(1, 2); + + v.push_back( S {} ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(); + + // clang-format on +} + +void testPointers() { + std::vector v; + v.push_back(new int(5)); + + std::vector> v2; + v2.push_back(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); + // Same here +} + +void testMakePair() { + std::vector> v; + // FIXME: add functionality to change calls of std::make_pair + v.push_back(std::make_pair(1, 2)); +} + +void testOtherCointainers() { + std::list l; + l.push_back(S(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: l.emplace_back(42, 41); + + std::deque d; + d.push_back(S(42)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: d.emplace_back(42); +}