-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang-tidy] Add detection in container's method except empty
in readability-container-size-empty
#154017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…eadability-container-size-empty`
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (flovent) Changes
Note: testcase about not reporting in Closes #152649 Full diff: https://github.com/llvm/llvm-project/pull/154017.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
index c4dc319f23c38..38c0330c02a7e 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -110,6 +110,25 @@ AST_MATCHER_P(UserDefinedLiteral, hasLiteral,
return false;
}
+AST_MATCHER_P(CXXMethodDecl, hasCanonicalDecl,
+ clang::ast_matchers::internal::Matcher<CXXMethodDecl>,
+ InnerMatcher) {
+ return InnerMatcher.matches(*Node.getCanonicalDecl(), Finder, Builder);
+}
+
+AST_POLYMORPHIC_MATCHER_P(
+ matchMemberName,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(MemberExpr, CXXDependentScopeMemberExpr),
+ std::string, MemberName) {
+ if (const auto *E = dyn_cast<MemberExpr>(&Node))
+ return E->getMemberDecl()->getName() == MemberName;
+
+ if (const auto *E = dyn_cast<CXXDependentScopeMemberExpr>(&Node))
+ return E->getMember().getAsString() == MemberName;
+
+ return false;
+}
+
} // namespace
using utils::isBinaryOrTernary;
@@ -140,9 +159,10 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
const auto ValidContainerNonTemplateType =
qualType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(ValidContainerRecord))));
- const auto ValidContainerTemplateType =
- qualType(hasUnqualifiedDesugaredType(templateSpecializationType(
- hasDeclaration(classTemplateDecl(has(ValidContainerRecord))))));
+ const auto ValidContainerTemplateType = qualType(hasUnqualifiedDesugaredType(
+ anyOf(templateSpecializationType(
+ hasDeclaration(classTemplateDecl(has(ValidContainerRecord)))),
+ injectedClassNameType(hasDeclaration(ValidContainerRecord)))));
const auto ValidContainer = qualType(
anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType));
@@ -155,6 +175,9 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
.bind("SizeBinaryOp")),
usedInBooleanContext());
+ const auto NotInEmptyMethodOfContainer = unless(
+ forCallable(cxxMethodDecl(hasCanonicalDecl(equalsBoundNode("empty")))));
+
Finder->addMatcher(
cxxMemberCallExpr(
argumentCountIs(0),
@@ -164,25 +187,23 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
.bind("MemberCallObject")),
callee(
cxxMethodDecl(hasAnyName("size", "length")).bind("SizeMethod")),
- WrongUse,
- unless(hasAncestor(
- cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ WrongUse, NotInEmptyMethodOfContainer)
.bind("SizeCallExpr"),
this);
Finder->addMatcher(
- callExpr(argumentCountIs(0),
- has(cxxDependentScopeMemberExpr(
- hasObjectExpression(
- expr(anyOf(hasType(ValidContainer),
- hasType(pointsTo(ValidContainer)),
- hasType(references(ValidContainer))))
- .bind("MemberCallObject")),
- anyOf(hasMemberName("size"), hasMemberName("length")))
- .bind("DependentExpr")),
- WrongUse,
- unless(hasAncestor(
- cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ callExpr(
+ argumentCountIs(0),
+ has(mapAnyOf(memberExpr, cxxDependentScopeMemberExpr)
+ .with(
+ hasObjectExpression(
+ expr(anyOf(hasType(ValidContainer),
+ hasType(pointsTo(ValidContainer)),
+ hasType(references(ValidContainer))))
+ .bind("MemberCallObject")),
+ anyOf(matchMemberName("size"), matchMemberName("length")))
+ .bind("MemberExpr")),
+ WrongUse, NotInEmptyMethodOfContainer)
.bind("SizeCallExpr"),
this);
@@ -217,8 +238,7 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
hasAnyOperatorName("==", "!="), hasOperands(WrongComparend, STLArg),
unless(allOf(hasLHS(hasType(ExcludedComparisonTypesMatcher)),
hasRHS(hasType(SameExcludedComparisonTypesMatcher)))),
- unless(hasAncestor(
- cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ NotInEmptyMethodOfContainer)
.bind("BinCmp"),
this);
}
@@ -382,9 +402,12 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
Diag << SizeMethod;
else if (const auto *DependentExpr =
Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>(
- "DependentExpr"))
+ "MemberExpr"))
Diag << DependentExpr->getMember();
- else
+ else if (const auto *ME =
+ Result.Nodes.getNodeAs<MemberExpr>("MemberExpr")) {
+ Diag << ME->getMemberNameInfo().getName();
+ } else
Diag << "unknown method";
Diag << Hint;
} else {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fd81b0d47e82b..a0ef1b6767cbe 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -202,7 +202,8 @@ Changes in existing checks
- Improved :doc:`readability-container-size-empty
<clang-tidy/checks/readability/container-size-empty>` check by correctly
- generating fix-it hints when size method is called from implicit ``this``.
+ generating fix-it hints when size method is called from implicit ``this``
+ and adding detection in container's method except ``empty``.
- Improved :doc:`readability-identifier-naming
<clang-tidy/checks/readability/identifier-naming>` check by ignoring
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 b1e68672a3a9a..decc9d2782218 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
@@ -908,3 +908,46 @@ class foo : public std::string{
};
}
+
+class ReportInContainerNonEmptyMethod {
+public:
+ int size() const;
+ bool empty() const;
+
+ void doit() {
+ if (!size()) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+ // CHECK-FIXES: if (empty())
+ }
+ }
+};
+
+template <typename T>
+class ReportInTemplateContainerNonEmptyMethod {
+public:
+ int size() const;
+ bool empty() const;
+
+ void doit() {
+ if (!size()) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+ // CHECK-FIXES: if (empty()) {
+ }
+ }
+};
+
+
+
+class ReportInContainerNonEmptyMethodCompare {
+public:
+ bool operator==(const ReportInContainerNonEmptyMethodCompare& other) const;
+ int size() const;
+ bool empty() const;
+
+ void doit() {
+ if (*this == ReportInContainerNonEmptyMethodCompare()) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object
+ // CHECK-FIXES: if (this->empty()) {
+ }
+ }
+};
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
readability-container-size-empty
skip check in container's all method, but onlyempty
method is nesscessary to skip because it's usually implemented bysize
, so this patch allows reporting in other methods.Note: testcase about not reporting in
empty
is already added in the past, so i only add testcases for other methods.Closes #152649