Index: clang-tidy/modernize/UseEmplaceCheck.h =================================================================== --- clang-tidy/modernize/UseEmplaceCheck.h +++ clang-tidy/modernize/UseEmplaceCheck.h @@ -11,6 +11,8 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H #include "../ClangTidy.h" +#include +#include namespace clang { namespace tidy { @@ -20,15 +22,19 @@ /// 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 ContainersWithPushBack; + std::vector SmartPointers; }; } // namespace modernize Index: clang-tidy/modernize/UseEmplaceCheck.cpp =================================================================== --- clang-tidy/modernize/UseEmplaceCheck.cpp +++ clang-tidy/modernize/UseEmplaceCheck.cpp @@ -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 @@ // + 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( + 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(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 @@ 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 Index: clang-tidy/utils/Matchers.h =================================================================== --- clang-tidy/utils/Matchers.h +++ clang-tidy/utils/Matchers.h @@ -40,8 +40,6 @@ 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; Index: docs/clang-tidy/checks/modernize-use-emplace.rst =================================================================== --- docs/clang-tidy/checks/modernize-use-emplace.rst +++ docs/clang-tidy/checks/modernize-use-emplace.rst @@ -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 `_, 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 @@ std::vector v; v.push_back(MyClass(21, 37)); - std::vector > w; - w.push_back(std::make_pair(21, 37)); + std::vector> w; + + w.push_back(std::pair(21, 37)); + w.push_back(std::make_pair(21L, 37L)); After: @@ -24,8 +35,10 @@ std::vector v; v.emplace_back(21, 37); - std::vector > w; - v.emplace_back(21, 37); + std::vector> 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 @@ 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 > v; - v.push_back(new int(5)); - auto *ptr = int; - v.push_back(ptr); + + std::vector> v; + v.push_back(std::unique_ptr(new int(0))); + auto *ptr = new int(1); + v.push_back(std::unique_ptr(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 @@ or if the argument would be converted via derived-to-base cast. This check requires C++11 of higher to run. - Index: test/clang-tidy/modernize-use-emplace.cpp =================================================================== --- test/clang-tidy/modernize-use-emplace.cpp +++ test/clang-tidy/modernize-use-emplace.cpp @@ -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 @@ -9,6 +12,7 @@ template void emplace_back(Args &&... args){}; + ~vector(); }; template class list { @@ -18,6 +22,7 @@ template void emplace_back(Args &&... args){}; + ~list(); }; template @@ -28,6 +33,7 @@ template void emplace_back(Args &&... args){}; + ~deque(); }; template @@ -54,10 +60,24 @@ template class unique_ptr { public: - unique_ptr(T *) {} + explicit unique_ptr(T *) {} + ~unique_ptr(); }; } // namespace std +namespace llvm { +template +class LikeASmallVector { +public: + void push_back(const T &) {} + void push_back(T &&) {} + + template + void emplace_back(Args &&... args){}; +}; + +} // llvm + void testInts() { std::vector v; v.push_back(42); @@ -72,6 +92,7 @@ Something(int a, int b = 41) {} Something() {} void push_back(Something); + int getInt() { return 42; } }; struct Convertable { @@ -103,6 +124,15 @@ // 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 @@ v.push_back(s); } +template +void dependOnElem() { + std::vector v; + v.push_back(ElemType(42)); +} + +template +void dependOnContainer() { + ContainerType v; + v.push_back(Something(42)); +} + +void callDependent() { + dependOnElem(); + dependOnContainer>(); +} + void test2() { std::vector v; v.push_back(Zoz(Something(21, 37))); @@ -130,12 +177,20 @@ v.push_back(getZoz(Something(1, 2))); } +struct GetPair { + std::pair getPair(); +}; 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); + 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> v2; v2.push_back(std::pair(Something(42, 42), Zoz(Something(21, 37)))); // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back @@ -206,14 +261,14 @@ v.push_back(new int(5)); std::vector> v2; - v2.push_back(new int(42)); + v2.push_back(std::unique_ptr(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(ptr)); // Same here } @@ -240,6 +295,11 @@ d.push_back(Something(42)); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back // CHECK-FIXES: d.emplace_back(42); + + llvm::LikeASmallVector 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 @@ // 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 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 v; + + v.push_back(WithDtor(42)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(42); +}