Index: clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt @@ -17,6 +17,7 @@ UseAutoCheck.cpp UseBoolLiteralsCheck.cpp UseDefaultCheck.cpp + UseEmplaceCheck.cpp UseNullptrCheck.cpp UseOverrideCheck.cpp Index: clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tools-extra/trunk/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" @@ -54,6 +55,7 @@ CheckFactories.registerCheck( "modernize-use-bool-literals"); CheckFactories.registerCheck("modernize-use-default"); + CheckFactories.registerCheck("modernize-use-emplace"); CheckFactories.registerCheck("modernize-use-nullptr"); CheckFactories.registerCheck("modernize-use-override"); } Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h +++ clang-tools-extra/trunk/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 looks 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-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -0,0 +1,104 @@ +//===--- 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" +#include "../utils/Matchers.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 careful + // 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", "llvm::SmallVector", + "std::list", "std::deque"))))); + + // 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")))); + + // Bitfields binds only to consts and emplace_back take it by universal ref. + auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts( + memberExpr(hasDeclaration(fieldDecl(matchers::isBitfield()))))); + + // We could have leak of resource. + auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr())); + auto constructingDerived = + hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase))); + + auto hasInitList = has(ignoringParenImpCasts(initListExpr())); + auto soughtConstructExpr = + cxxConstructExpr( + unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument, + newExprAsArgument, constructingDerived, + has(materializeTemporaryExpr(hasInitList))))) + .bind("ctor"); + auto hasConstructExpr = has(ignoringParenImpCasts(soughtConstructExpr)); + + auto ctorAsArgument = materializeTemporaryExpr( + anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr)))); + + Finder->addMatcher( + cxxMemberCallExpr(callPushBack, has(ctorAsArgument)).bind("call"), this); +} + +void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs("call"); + const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor"); + + auto FunctionNameSourceRange = CharSourceRange::getCharRange( + Call->getExprLoc(), Call->getArg(0)->getExprLoc()); + + auto Diag = diag(Call->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)); + + Diag << FixItHint::CreateRemoval(CtorCallSourceRange) + << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + CallParensRange.getEnd(), + CallParensRange.getEnd().getLocWithOffset(1))); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h +++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h @@ -45,6 +45,8 @@ Node, Finder->getASTContext()); } +AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); } + } // namespace matchers } // namespace tidy } // namespace clang Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/trunk/docs/ReleaseNotes.rst +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst @@ -220,6 +220,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: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst @@ -0,0 +1,69 @@ +.. title:: clang-tidy - modernize-use-emplace + +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. + +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 +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++. + +Check also fires if any argument of constructor call would be: +- bitfield (bitfields can't bind to rvalue/universal reference) +- ``new`` expression (to avoid leak) +or if the argument would be converted via derived-to-base cast. + +This check requires C++11 of higher to run. + Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp +++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp @@ -0,0 +1,338 @@ +// 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}); + v.push_back(42.0); + int z; + v.push_back(z); +} + +struct Something { + Something(int a, int b = 41) {} + Something() {} + void push_back(Something); +}; + +struct Convertable { + operator Something() { return Something{}; } +}; + +struct Zoz { + Zoz(Something, int = 42) {} +}; + +Zoz getZoz(Something s) { return Zoz(s); } + +void test_Something() { + std::vector v; + + v.push_back(Something(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(Something{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(1, 2); + + v.push_back(Something()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(); + + v.push_back(Something{}); + // 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); + + Something 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(Something(21, 37))); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(Something(21, 37)); + + v.push_back(Zoz(Something(21, 37), 42)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(Something(21, 37), 42); + + v.push_back(getZoz(Something(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(Something(42, 42), Zoz(Something(21, 37)))); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back + // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37))); +} + +struct Base { + Base(int, int *, int = 42); +}; + +struct Derived : Base { + Derived(int *, Something) : Base(42, nullptr) {} +}; + +void testDerived() { + std::vector v; + v.push_back(Derived(nullptr, Something{})); +} + +void testNewExpr() { + std::vector v; + v.push_back(Derived(new int, Something{})); +} + +void testSpaces() { + std::vector v; + + // clang-format off + + v.push_back(Something(1, //arg1 + 2 // arg2 + ) // Something + ); + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(1, //arg1 + // CHECK-FIXES: 2 // arg2 + // CHECK-FIXES: // Something + // CHECK-FIXES: ); + + v.push_back( Something (1, 2) ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(1, 2 ); + + v.push_back( Something {1, 2} ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(1, 2 ); + + v.push_back( Something {} ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back( ); + + v.push_back( + Something(1, 2) ); + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(1, 2 ); + + std::vector v2; + v2.push_back( + Base(42, nullptr)); + // CHECK-MESSAGES: :[[@LINE-2]]:6: warning: use emplace_back + // CHECK-FIXES: v2.emplace_back(42, nullptr); + + // 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)); + + // FIXME: This is not a bug, but call of make_pair should be removed in the + // future. This one matches because the return type of make_pair is different + // than the pair itself. + v.push_back(std::make_pair(42LL, 13)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13)); +} + +void testOtherCointainers() { + std::list l; + l.push_back(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: l.emplace_back(42, 41); + + std::deque d; + d.push_back(Something(42)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: d.emplace_back(42); +} + +class IntWrapper { +public: + IntWrapper(int x) : value(x) {} + IntWrapper operator+(const IntWrapper other) const { + return IntWrapper(value + other.value); + } + +private: + int value; +}; + +void testMultipleOpsInPushBack() { + std::vector v; + v.push_back(IntWrapper(42) + IntWrapper(27)); +} + +// Macro tests. +#define PUSH_BACK_WHOLE(c, x) c.push_back(x) +#define PUSH_BACK_NAME push_back +#define PUSH_BACK_ARG(x) (x) +#define SOME_OBJ Something(10) +#define MILLION 3 +#define SOME_WEIRD_PUSH(v) v.push_back(Something( +#define OPEN ( +#define CLOSE ) +void macroTest() { + std::vector v; + Something s; + + PUSH_BACK_WHOLE(v, Something(5, 6)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use emplace_back + + v.PUSH_BACK_NAME(Something(5)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + + v.push_back PUSH_BACK_ARG(Something(5, 6)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + + v.push_back(SOME_OBJ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + + v.push_back(Something(MILLION)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(MILLION); + + // clang-format off + v.push_back( Something OPEN 3 CLOSE ); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // clang-format on + PUSH_BACK_WHOLE(s, Something(1)); +} + +struct A { + int value1, value2; +}; + +struct B { + B(A) {} +}; + +struct C { + int value1, value2, value3; +}; + +void testAggregation() { + // This should not be noticed or fixed; after the correction, the code won't + // compile. + + std::vector v; + v.push_back(A({1, 2})); + + std::vector vb; + vb.push_back(B({10, 42})); +} + +struct Bitfield { + unsigned bitfield : 1; + unsigned notBitfield; +}; + +void testBitfields() { + std::vector v; + Bitfield b; + v.push_back(Something(42, b.bitfield)); + v.push_back(Something(b.bitfield)); + + v.push_back(Something(42, b.notBitfield)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(42, b.notBitfield); + int var; + v.push_back(Something(42, var)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(42, var); +}