llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Andrew Gaul (gaul) <details> <summary>Changes</summary> The check only recognizes insertion methods named `push_back`, `push` and `push_front`, and its fix-its call `emplace_back`, `emplace` and `emplace_front` instead. Containers outside the standard library often use other names: Mozilla's `nsTArray` has `AppendElement` and `EmplaceBack`, and `mozilla::Vector` has `append` and `emplaceBack`. The new option takes semicolon-separated `function=replacement` pairs, such as `Vector::append=emplaceBack`, where the function is named as in `EmplacyFunctions`. Such methods are often templates that take a forwarding reference, so the temporary passed to them need not be of the element type, and the emplacement method would then call a different constructor. Calls are therefore only reported when the temporary is of the container's `value_type`, which, like the method itself, may be declared in a base class of the container. --- Full diff: https://github.com/llvm/llvm-project/pull/237768.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp (+94-20) - (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h (+3) - (modified) clang-tools-extra/docs/ReleaseNotes.md (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst (+15) - (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-push-like-functions.cpp (+106) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index 8ec123fc476ff4..1faf622445557c 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -12,17 +12,8 @@ using namespace clang::ast_matchers; namespace clang::tidy::modernize { -namespace { -AST_MATCHER_P(InitListExpr, initCountLeq, unsigned, N) { - return Node.getNumInits() <= N; -} - -// Identical to hasAnyName, except it does not take template specifiers into -// account. This is used to match the functions names as in -// DefaultEmplacyFunctions below without caring about the template types of the -// containers. -AST_MATCHER_P(NamedDecl, hasAnyNameIgnoringTemplates, std::vector<StringRef>, - Names) { +// Returns the fully qualified name of Node without template specifiers. +static std::string getNameIgnoringTemplates(const NamedDecl &Node) { const std::string FullName = "::" + Node.getQualifiedNameAsString(); // This loop removes template specifiers by only keeping characters not within @@ -37,16 +28,33 @@ AST_MATCHER_P(NamedDecl, hasAnyNameIgnoringTemplates, std::vector<StringRef>, --Depth; else if (Depth == 0) FullNameTrimmed.append(1, Character); + return FullNameTrimmed; +} + +// Checks whether a name returned by getNameIgnoringTemplates matches Pattern. +// This is taken from HasNameMatcher::matchesNodeFullSlow in +// clang/lib/ASTMatchers/ASTMatchersInternal.cpp. +static bool nameMatches(StringRef FullNameTrimmed, StringRef Pattern) { + if (Pattern.starts_with("::")) + return FullNameTrimmed == Pattern; + return FullNameTrimmed.ends_with(Pattern) && + FullNameTrimmed.drop_back(Pattern.size()).ends_with("::"); +} + +namespace { +AST_MATCHER_P(InitListExpr, initCountLeq, unsigned, N) { + return Node.getNumInits() <= N; +} - // This loop is taken from HasNameMatcher::matchesNodeFullSlow in - // clang/lib/ASTMatchers/ASTMatchersInternal.cpp and checks whether - // FullNameTrimmed matches any of the given Names. - const StringRef FullNameTrimmedRef = FullNameTrimmed; +// Identical to hasAnyName, except it does not take template specifiers into +// account. This is used to match the functions names as in +// DefaultEmplacyFunctions below without caring about the template types of the +// containers. +AST_MATCHER_P(NamedDecl, hasAnyNameIgnoringTemplates, std::vector<StringRef>, + Names) { + const std::string FullNameTrimmed = getNameIgnoringTemplates(Node); return llvm::any_of(Names, [&](const StringRef Pattern) { - if (Pattern.starts_with("::")) - return FullNameTrimmed == Pattern; - return FullNameTrimmedRef.ends_with(Pattern) && - FullNameTrimmedRef.drop_back(Pattern.size()).ends_with("::"); + return nameMatches(FullNameTrimmed, Pattern); }); } @@ -142,7 +150,21 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context) TupleMakeFunctions(utils::options::parseStringList( Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))), EmplacyFunctions(utils::options::parseStringList( - Options.get("EmplacyFunctions", DefaultEmplacyFunctions))) {} + Options.get("EmplacyFunctions", DefaultEmplacyFunctions))), + PushLikeFunctions(utils::options::parseStringList( + Options.get("PushLikeFunctions", ""))) { + for (const StringRef Entry : PushLikeFunctions) { + const auto [Function, Replacement] = Entry.split('='); + if (Function.trim().empty() || Replacement.trim().empty()) { + configurationDiag("invalid entry '%0' in option '%1.PushLikeFunctions'; " + "expected 'function=replacement'") + << Entry << Name; + continue; + } + PushLikeFunctionNames.push_back(Function.trim()); + PushLikeReplacements.push_back(Replacement.trim()); + } +} void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { // FIXME: Bunch of functionality that could be easily added: @@ -296,6 +318,30 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { unless(isInTemplateInstantiation())) .bind("emplacy_call")), this); + + if (PushLikeFunctionNames.empty()) + return; + + // A push_back-like method may be a template that accepts any type, so require + // the temporary to be of the container's value_type. Both the method and + // value_type may be declared in a base class of the container. + const auto HasValueType = has(typedefNameDecl( + hasName("value_type"), + hasType(hasCanonicalType(recordType().bind("value_type"))))); + const auto CallPushLike = cxxMemberCallExpr( + hasDeclaration( + functionDecl(hasAnyNameIgnoringTemplates(PushLikeFunctionNames))), + on(hasTypeOrPointeeType(hasCanonicalType(hasDeclaration( + cxxRecordDecl(isSameOrDerivedFrom(cxxRecordDecl(HasValueType))))))), + argumentCountIs(1)); + + Finder->addMatcher( + traverse(TK_AsIs, cxxMemberCallExpr( + CallPushLike, + HasConstructExprWithValueTypeTypeAsLastArgument, + unless(isInTemplateInstantiation())) + .bind("push_like_call")), + this); } void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { @@ -304,6 +350,8 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { const auto *PushCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_call"); const auto *PushFrontCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call"); + const auto *PushLikeCall = + Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_like_call"); const auto *EmplacyCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call"); const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor"); @@ -318,6 +366,8 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { return PushCall; if (PushFrontCall) return PushFrontCall; + if (PushLikeCall) + return PushLikeCall; return EmplacyCall; }(); @@ -328,6 +378,21 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange()) return; + StringRef PushLikeReplacement; + if (PushLikeCall) { + const std::string Name = + getNameIgnoringTemplates(*PushLikeCall->getMethodDecl()); + for (const auto [Function, Replacement] : + llvm::zip_equal(PushLikeFunctionNames, PushLikeReplacements)) { + if (nameMatches(Name, Function)) { + PushLikeReplacement = Replacement; + break; + } + } + assert(!PushLikeReplacement.empty() && + "No push_back-like function matched"); + } + const auto FunctionNameSourceRange = CharSourceRange::getCharRange( Call->getExprLoc(), Call->getArg(0)->getExprLoc()); @@ -337,10 +402,14 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { : CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(), "unnecessary temporary object created while calling %0") + : PushLikeCall + ? diag(Call->getExprLoc(), "use %0 instead of %1") : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 " "instead of push%select{|_back|_front}0"); if (EmplacyCall) Diag << Call->getMethodDecl()->getName(); + else if (PushLikeCall) + Diag << PushLikeReplacement << Call->getMethodDecl()->getName(); else if (PushCall) Diag << 0; else if (PushBackCall) @@ -363,6 +432,9 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { const char *EmplacePrefix = MakeCall ? "emplace_front" : "emplace_front("; Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix); + } else if (PushLikeCall) { + Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, + (PushLikeReplacement + "(").str()); } const SourceRange CallParensRange = @@ -415,6 +487,8 @@ void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { utils::options::serializeStringList(TupleMakeFunctions)); Options.store(Opts, "EmplacyFunctions", utils::options::serializeStringList(EmplacyFunctions)); + Options.store(Opts, "PushLikeFunctions", + utils::options::serializeStringList(PushLikeFunctions)); } } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h index a7ad5bb166b6bc..3dee05a55dec77 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h @@ -41,6 +41,9 @@ class UseEmplaceCheck : public ClangTidyCheck { const std::vector<StringRef> TupleTypes; const std::vector<StringRef> TupleMakeFunctions; const std::vector<StringRef> EmplacyFunctions; + const std::vector<StringRef> PushLikeFunctions; + std::vector<StringRef> PushLikeFunctionNames; + std::vector<StringRef> PushLikeReplacements; }; } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 679ca9ecbb4a39..68bd3b3d320671 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -301,6 +301,12 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/modernize/use-designated-initializers>` when analyzing malformed code with nested classes and ambiguous initializer. +- Improved {doc}`modernize-use-emplace + <clang-tidy/checks/modernize/use-emplace>` check by adding the + {option}`PushLikeFunctions` option to support containers whose insertion + and emplacement methods have other names, such as `append` and + `emplaceBack`. + - Improved {doc}`modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default>` check to also capture default constructor with explicit default-initialization of parents. 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 f1031f69a94679..b1dd364affbec8 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 @@ -24,6 +24,9 @@ respectively. The default values for these options are as follows: * :option:`ContainersWithPushFront`: ``std::forward_list``, ``std::list``, and ``std::deque``. +Insertion methods with other names, such as ``append``, can be added with the +:option:`PushLikeFunctions` option. + This check also reports when an ``emplace``-like method is improperly used, for example using ``emplace_back`` while also calling a constructor. This creates a temporary that requires at best a move and at worst a copy. Almost @@ -169,6 +172,18 @@ Options the check will report when a temporary is unnecessarily created. All STL containers with such member functions are supported by default. +.. option:: PushLikeFunctions + + Semicolon-separated list of `function=replacement` pairs, where `function` + is a container and a ``push_back``-like method of it, without their + template parameters, and `replacement` is the ``emplace``-like method that + should be called instead. Example: ``MyVector::append=emplace_append``. A + method inherited from a base class is named with the base class. Since + such a method may be a template that accepts arguments of any type, the + check only reports a call whose argument is an explicitly-constructed + temporary of the container's ``value_type``, which may also be declared in + a base class. Default is an empty string. + Example ^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-push-like-functions.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-push-like-functions.cpp new file mode 100644 index 00000000000000..6f8900ac4045d1 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-push-like-functions.cpp @@ -0,0 +1,106 @@ +// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: {modernize-use-emplace.PushLikeFunctions: \ +// RUN: '::Vector::append=emplaceBack; \ +// RUN: Vector::infallibleAppend=infallibleEmplaceBack; \ +// RUN: ArrayBase::AppendElement=EmplaceBack; \ +// RUN: NoValueType::append=emplaceBack; \ +// RUN: invalid'}}" + +// CHECK-MESSAGES: warning: invalid entry 'invalid' in option 'modernize-use-emplace.PushLikeFunctions'; expected 'function=replacement' [clang-tidy-config] + +template <typename T> +class Vector { +public: + using value_type = T; + template <typename U> bool append(U &&); + template <typename... Args> bool emplaceBack(Args &&...); + template <typename U> void infallibleAppend(U &&); + template <typename... Args> void infallibleEmplaceBack(Args &&...); + template <typename U> bool appendOther(U &&); +}; + +template <typename E> +class ArrayBase { +public: + typedef E value_type; + template <typename Item> E *AppendElement(Item &&); + template <typename Item> E *AppendElement(Item &&, int); + template <typename... Args> E *EmplaceBack(Args &&...); +}; + +template <typename E> +class Array : public ArrayBase<E> { +public: + using typename ArrayBase<E>::value_type; + using ArrayBase<E>::AppendElement; +}; + +template <typename T> +class NoValueType { +public: + template <typename U> bool append(U &&); + template <typename... Args> bool emplaceBack(Args &&...); +}; + +struct Something { + Something(int A, int B = 41) {} + Something() {} +}; + +struct Wrapper { + Wrapper(Something) {} +}; + +void testVector() { + Vector<Something> V; + V.append(Something(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplaceBack instead of append [modernize-use-emplace] + // CHECK-FIXES: V.emplaceBack(1, 2); + + V.append(Something{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplaceBack instead of append + // CHECK-FIXES: V.emplaceBack(1, 2); + + V.append(Something()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplaceBack instead of append + // CHECK-FIXES: V.emplaceBack(); + + V.infallibleAppend(Something(3)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use infallibleEmplaceBack instead of infallibleAppend + // CHECK-FIXES: V.infallibleEmplaceBack(3); + + V.appendOther(Something(1, 2)); + + Something S; + V.append(S); + + Vector<Wrapper> W; + W.append(Something(1, 2)); +} + +void testArray(Array<Something> *P) { + Array<Something> A; + A.AppendElement(Something(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use EmplaceBack instead of AppendElement + // CHECK-FIXES: A.EmplaceBack(1, 2); + + P->AppendElement(Something(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use EmplaceBack instead of AppendElement + // CHECK-FIXES: P->EmplaceBack(1, 2); + + A.AppendElement(Something(1, 2), 0); +} + +void testNoValueType() { + NoValueType<Something> N; + N.append(Something(1, 2)); +} + +template <typename T> +void testDependent() { + Vector<T> V; + V.append(T(1, 2)); +} + +void callDependent() { testDependent<Something>(); } `````````` </details> https://github.com/llvm/llvm-project/pull/237768 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
