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,18 @@ /// 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, SmartPointers; }; } // namespace modernize Index: clang-tidy/modernize/UseEmplaceCheck.cpp =================================================================== --- clang-tidy/modernize/UseEmplaceCheck.cpp +++ clang-tidy/modernize/UseEmplaceCheck.cpp @@ -8,18 +8,48 @@ //===----------------------------------------------------------------------===// #include "UseEmplaceCheck.h" -#include "../utils/Matchers.h" - +#include "../utils/OptionsUtils.h" using namespace clang::ast_matchers; namespace clang { namespace tidy { namespace modernize { +namespace { + +llvm::Optional> +getHasAnyName(const std::vector &Names) { + llvm::Optional> HasNameMatcher; + + for (const auto &ClassName : Names) { + const auto HasName = hasName(ClassName); + HasNameMatcher = HasNameMatcher ? anyOf(*HasNameMatcher, HasName) : HasName; + } + return HasNameMatcher; +} + +const std::string DefaultContainersWithPushBack = + "::std::vector; ::std::list; ::std::deque"; +const std::string DefaultSmartPointers = + "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr"; +} // namespace + +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; + auto HasContainerName = getHasAnyName(ContainersWithPushBack); + // No containers to match. + if (!HasContainerName) + return; + // FIXME: Bunch of functionality that could be easily added: // + add handling of `push_front` for std::forward_list, std::list // and std::deque. @@ -29,42 +59,48 @@ // 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"))))); + auto callPushBack = + cxxMemberCallExpr(hasDeclaration(functionDecl(hasName("push_back"))), + on(hasType(cxxRecordDecl(*HasContainerName)))); + + auto hasSmartPointerName = getHasAnyName(SmartPointers); // 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 = + hasSmartPointerName + ? hasDeclaration(cxxConstructorDecl(ofClass(*hasSmartPointerName))) + : hasDeclaration(unless(anything())); // 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()))))); // We could have leak of resource. - auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr())); + auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr())); auto constructingDerived = hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase))); - auto hasInitList = has(ignoringParenImpCasts(initListExpr())); + 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))))) .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 +125,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 @@ -4,8 +4,12 @@ ===================== 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. +container, but the element is constructed temporarily. + +By default only ``std::vector``, ``std::deque``, ``std::list`` are considered. +This list can be modified by passing a `;` separated list of class names using the `ContainersWithPushBack` +option. + Before: @@ -14,8 +18,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 +30,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 +53,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) 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); @@ -117,6 +137,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 +167,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 +251,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 +285,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 +386,16 @@ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back // CHECK-FIXES: v.emplace_back(42, var); } + +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); +}