Skip to content

[clang-tidy] support string::contains #110351

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

[clang-tidy] support string::contains #110351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b98e9a4
[clang-tidy] support string::contains
dl8sd11 Sep 28, 2024
e42c356
[clang-tidy] fix string::find(s, 0)
dl8sd11 Oct 5, 2024
9d44ee0
[clang-tidy] add string::contains
dl8sd11 Oct 5, 2024
3625221
[clang-tidy] adopt mock headers and add string_view tests
dl8sd11 Oct 5, 2024
608e883
[clang-tidy] add comment expain 2 arguments
dl8sd11 Oct 5, 2024
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
22 changes: 16 additions & 6 deletions clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@

#include "ContainerContainsCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Expr.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"

using namespace clang::ast_matchers;

Expand All @@ -30,9 +32,15 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
ofClass(cxxRecordDecl(HasContainsMatchingParamType)))))
.bind("call");

const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));

const auto FindCall =
cxxMemberCallExpr(
argumentCountIs(1),
anyOf(argumentCountIs(1),
allOf(argumentCountIs(2), // string::find takes two arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allOf(argumentCountIs(2), // string::find takes two arguments
// `find` for string-like classes often has a second parameter,
// indicating where to start the search. We match if it's
// the default zero or explicitly equal to zero.
allOf(argumentCountIs(2),

I think a little more detail is good here.

hasArgument(1, anyOf(cxxDefaultArgExpr(),
ignoringParenImpCasts(Literal0))))),
callee(cxxMethodDecl(
hasName("find"),
hasParameter(0, hasType(hasUnqualifiedDesugaredType(
Expand All @@ -48,8 +56,8 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
// before EndCall so 'parameterType' is properly bound.
ofClass(cxxRecordDecl(HasContainsMatchingParamType)))));

const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));
const auto StringNpos = anyOf(declRefExpr(to(varDecl(hasName("npos")))),
memberExpr(member(hasName("npos"))));

auto AddSimpleMatcher = [&](auto Matcher) {
Finder->addMatcher(
Expand Down Expand Up @@ -94,12 +102,14 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
.bind("negativeComparison"));

// Find membership tests based on `find() == end()`.
// Find membership tests based on `find() == end()` or `find() == npos`.
AddSimpleMatcher(
binaryOperator(hasOperatorName("!="), hasOperands(FindCall, EndCall))
binaryOperator(hasOperatorName("!="),
hasOperands(FindCall, anyOf(EndCall, StringNpos)))
.bind("positiveComparison"));
AddSimpleMatcher(
binaryOperator(hasOperatorName("=="), hasOperands(FindCall, EndCall))
binaryOperator(hasOperatorName("=="),
hasOperands(FindCall, anyOf(EndCall, StringNpos)))
.bind("negativeComparison"));
}

Expand Down
7 changes: 7 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,16 @@ struct basic_string {
size_type find(const _Type& str, size_type pos = 0) const;
size_type find(const C* s, size_type pos = 0) const;
size_type find(const C* s, size_type pos, size_type n) const;
size_type find(C ch, size_type pos = 0) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
size_type find(C ch, size_type pos = 0) const;
size_type find(C ch, size_type pos = 0) const;
template<class StringViewLike>
size_type find(const StringViewLike& t, size_type pos = 0) const;

Let's cover them all if possible.
This one might cause issues, and is probably not all that common, but let's see.


size_type rfind(const _Type& str, size_type pos = npos) const;
size_type rfind(const C* s, size_type pos, size_type count) const;
size_type rfind(const C* s, size_type pos = npos) const;
size_type rfind(C ch, size_type pos = npos) const;

bool contains(const C *s) const;
bool contains(C s) const;
Comment on lines +60 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool contains(const C *s) const;
bool contains(C s) const;
bool contains(const C *s) const;
bool contains(C s) const;
bool contains(basic_string_view<C, T> sv) const;


_Type& insert(size_type pos, const _Type& str);
_Type& insert(size_type pos, const C* s);
_Type& insert(size_type pos, const C* s, size_type n);
Expand Down Expand Up @@ -104,6 +108,9 @@ struct basic_string_view {
size_type rfind(const C* s, size_type pos, size_type count) const;
size_type rfind(const C* s, size_type pos = npos) const;

bool contains(const C *s) const;
bool contains(C s) const;
Comment on lines +111 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool contains(const C *s) const;
bool contains(C s) const;
bool contains(const C *s) const;
bool contains(C s) const;
bool contains(basic_string_view sv) const;


constexpr bool starts_with(basic_string_view sv) const noexcept;
constexpr bool starts_with(C ch) const noexcept;
constexpr bool starts_with(const C* s) const;
Expand Down
33 changes: 32 additions & 1 deletion clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t
// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t -- \
// RUN: -- -isystem %clang_tidy_headers

#include <string>

// Some *very* simplified versions of `map` etc.
namespace std {
Expand Down Expand Up @@ -29,6 +32,8 @@ struct multimap {
bool contains(const Key &K) const;
};



} // namespace std

// Check that we detect various common ways to check for membership
Expand Down Expand Up @@ -453,3 +458,29 @@ void testOperandPermutations(std::map<int, int>& Map) {
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (!Map.contains(0)) {};
}

void testStringNops(std::string Str, std::string SubStr, std::string_view StrView) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void testStringNops(std::string Str, std::string SubStr, std::string_view StrView) {
void testStringNpos(std::string Str, std::string SubStr, std::string_view StrView) {

Just a typo.

if (Str.find("test") == std::string::npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (!Str.contains("test")) {};

if (Str.find('c') != std::string::npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (Str.contains('c')) {};

if (Str.find('c') != Str.npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (Str.contains('c')) {};

if (StrView.find("test") == std::string::npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (!StrView.contains("test")) {};

if (StrView.find('c') != std::string::npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (StrView.contains('c')) {};

if (StrView.find('c') != StrView.npos) {};
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (StrView.contains('c')) {};
}