Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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),
Expand All @@ -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);

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
}
}
};