llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: MitraGulla <details> <summary>Changes</summary> ## Summary The check can suggest replacing `length()` with `empty()` for `std::match_results`, but these two calls don't always mean the same thing. This change prevents the check from suggesting `empty()` when `length()` is called on `std::match_results`. Added tests for its different aliases and kept the existing suggestions for other containers. ## Testing - Ran `clang-format` on the modified C++ files. - Ran the `container-size-empty.cpp` lit test successfully. --- Full diff: https://github.com/llvm/llvm-project/pull/234905.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp (+20-2) - (modified) clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h (+1) - (modified) clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.md (+7) - (added) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/regex (+23) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp (+115) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp index 2db5385052843f..ea7a79a55a07d2 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -135,10 +135,14 @@ using utils::isBinaryOrTernary; ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), + ExcludedLengthTypes(utils::options::parseStringList( + Options.get("ExcludedLengthTypes", "::std::match_results"))), ExcludedComparisonTypes(utils::options::parseStringList( Options.get("ExcludedComparisonTypes", "::std::array"))) {} void ContainerSizeEmptyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "ExcludedLengthTypes", + utils::options::serializeStringList(ExcludedLengthTypes)); Options.store(Opts, "ExcludedComparisonTypes", utils::options::serializeStringList(ExcludedComparisonTypes)); } @@ -168,6 +172,15 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { const auto ValidContainer = qualType( anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType)); + const auto ExcludedLengthTypesMatcher = qualType( + anyOf(hasDeclaration(cxxRecordDecl( + matchers::matchesAnyListedRegexName(ExcludedLengthTypes))), + hasCanonicalType(hasDeclaration(cxxRecordDecl( + matchers::matchesAnyListedRegexName(ExcludedLengthTypes)))))); + const auto ExcludedLengthExprType = qualType( + anyOf(ExcludedLengthTypesMatcher, pointsTo(ExcludedLengthTypesMatcher), + references(ExcludedLengthTypesMatcher))); + const auto WrongUse = anyOf(hasParent(binaryOperator( isComparisonOperator(), @@ -189,7 +202,9 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { argumentCountIs(0), on(ValidContainerExpr), callee( cxxMethodDecl(hasAnyName("size", "length")).bind("SizeMethod")), - WrongUse, NotInEmptyMethodOfContainer) + WrongUse, NotInEmptyMethodOfContainer, + unless(allOf(callee(cxxMethodDecl(hasName("length"))), + on(expr(hasType(ExcludedLengthExprType)))))) .bind("SizeCallExpr"), this); @@ -198,7 +213,10 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { has(mapAnyOf(memberExpr, cxxDependentScopeMemberExpr) .with(hasObjectExpression(ValidContainerExpr), anyOf(matchMemberName("size"), - matchMemberName("length"))) + matchMemberName("length")), + unless(allOf(matchMemberName("length"), + hasObjectExpression(expr(hasType( + ExcludedLengthExprType)))))) .bind("MemberExpr")), WrongUse, NotInEmptyMethodOfContainer) .bind("SizeCallExpr"), diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h index 6d984d29ada66b..0d968c31ec7885 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h +++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h @@ -37,6 +37,7 @@ class ContainerSizeEmptyCheck : public ClangTidyCheck { } private: + std::vector<StringRef> ExcludedLengthTypes; std::vector<StringRef> ExcludedComparisonTypes; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.md b/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.md index 89b5f9fbbc886d..418349034c3576 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.md +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.md @@ -33,3 +33,10 @@ objects of the same type. If a class is listed here, the check will not suggest using `empty()` instead of such comparisons for objects of that class. Default is `::std::array`. ``` + +```{option} ExcludedLengthTypes +A semicolon-separated list of regular expressions matching class names for +which the check will not suggest replacing calls to `length()` with +`empty()`. This is useful for types where `length()` and `empty()` have +different semantics. Default is `::std::match_results`. +``` diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/regex b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/regex new file mode 100644 index 00000000000000..f293fde4ee3dce --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/regex @@ -0,0 +1,23 @@ +#ifndef TEST_REGEX +#define TEST_REGEX + +namespace std { + +template <typename BidirectionalIterator> +class match_results { +public: + using size_type = unsigned long; + + size_type length(size_type sub = 0) const; + size_type size() const; + bool empty() const; +}; + +using smatch = match_results<const char *>; +using cmatch = match_results<const char *>; +using wsmatch = match_results<const wchar_t *>; +using wcmatch = match_results<const wchar_t *>; + +} // namespace std + +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp index 79c42c213002f5..e1a63e64527536 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp @@ -3,6 +3,7 @@ // RUN: -- -fno-delayed-template-parsing #include <string> #include <memory> +#include <regex> namespace std { template <typename T> struct vector { @@ -35,6 +36,120 @@ auto empty(const C& c) -> decltype(c.empty()); } +void testRegexMatchResults(std::smatch &sm, std::cmatch &cm, + std::wsmatch &wsm, std::wcmatch &wcm) { + if (sm.length()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + if (!sm.length()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + if (sm.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!sm.empty()) {} + + if (sm.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + if (cm.length()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + if (cm.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!cm.empty()) {} + if (cm.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + if (wsm.length()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + if (wsm.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!wsm.empty()) {} + if (wsm.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + if (wcm.length()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + if (wcm.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!wcm.empty()) {} + if (wcm.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + // length() must also be excluded when compared with zero. + if (sm.length() == 0) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + if (sm.length() != 0) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + // size() remains safe to replace with empty(). + if (!sm.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (sm.empty()) {} + + if (sm.size() == 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (sm.empty()) {} + + if (sm.size() != 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!sm.empty()) {} +} + +void testOrdinaryContainers(std::vector<int> &vect, std::string &str) { + if (vect.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!vect.empty()) {} + + if (!vect.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (vect.empty()) {} + + if (vect.size() == 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (vect.empty()) {} + + if (vect.size() != 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!vect.empty()) {} + + if (vect.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: + + if (str.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!str.empty()) {} + + if (!str.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (str.empty()) {} + + if (str.length()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty] + // CHECK-FIXES: if (!str.empty()) {} + + if (!str.length()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty] + // CHECK-FIXES: if (str.empty()) {} + + if (str.size() == 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (str.empty()) {} + + if (str.length() == 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty] + // CHECK-FIXES: if (str.empty()) {} + + if (str.size() != 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-FIXES: if (!str.empty()) {} + + if (str.length() != 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty] + // CHECK-FIXES: if (!str.empty()) {} + + if (str.empty()) {} + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:{{[0-9]+}}: warning: +} + template <typename T> class TemplatedContainer { public: `````````` </details> https://github.com/llvm/llvm-project/pull/234905 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
