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 1 commit
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
18 changes: 14 additions & 4 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 @@ -9,6 +9,7 @@
#include "ContainerContainsCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"

using namespace clang::ast_matchers;

Expand All @@ -32,7 +33,8 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {

const auto FindCall =
cxxMemberCallExpr(
argumentCountIs(1),
anyOf(argumentCountIs(1),
allOf(argumentCountIs(2), hasArgument(1, cxxDefaultArgExpr()))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment here saying std::string and std::string_view take an optional pos argument on where to start the search. Also match ignoringParenImpCasts(integerLiteral(0))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to match the two arguments but I fail to remove the second argument in the Fixit hint. Could you suggest how to get the location after the first argument. I tried binding the first argument (e.g. "test"), but the source range seems to be the first character (") instead of the whole argument ("test").

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering str.find('a') != std::string::npos. Instead of deleting stuff from ) to npos, you could replace everything from after 'a' to the end of the comparison with ), that should work. I wrote something like this here:

llvm-project/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp

Lines 159 to 165 in e8f01b0

// Remove possible arguments after search expression and ' [!=]= 0' suffix.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(
Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
*Result.SourceManager, getLangOpts()),
ComparisonExpr->getEndLoc()),
")");

callee(cxxMethodDecl(
hasName("find"),
hasParameter(0, hasType(hasUnqualifiedDesugaredType(
Expand All @@ -51,6 +53,12 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));

const auto StringLikeClass = cxxRecordDecl(
hasAnyName("::std::basic_string", "::std::basic_string_view",
"::absl::string_view"));
const auto StringNpos = declRefExpr(
to(varDecl(hasName("npos"), hasDeclContext(StringLikeClass))));

auto AddSimpleMatcher = [&](auto Matcher) {
Finder->addMatcher(
traverse(TK_IgnoreUnlessSpelledInSource, std::move(Matcher)), this);
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
49 changes: 49 additions & 0 deletions 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
Expand Up @@ -29,6 +29,43 @@ struct multimap {
bool contains(const Key &K) const;
};

using size_t = decltype(sizeof(int));

// Lightweight standin for std::string_view.
template <typename C>
class basic_string_view {
public:
basic_string_view();
basic_string_view(const basic_string_view &);
basic_string_view(const C *);
~basic_string_view();
int find(basic_string_view s, int pos = 0);
int find(const C *s, int pos = 0);
int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
static constexpr size_t npos = -1;
};
typedef basic_string_view<char> string_view;

// Lightweight standin for std::string.
template <typename C>
class basic_string {
public:
basic_string();
basic_string(const basic_string &);
basic_string(const C *);
~basic_string();
int find(basic_string s, int pos = 0);
int find(const C *s, int pos = 0);
int find(const C *s, int pos, int n);
int find(char c, int pos = 0);
bool contains(const C *s) const;
bool contains(C s) const;
bool contains(basic_string_view<C> s) const;
static constexpr size_t npos = -1;
};
typedef basic_string<char> string;

} // namespace std

// Check that we detect various common ways to check for membership
Expand Down Expand Up @@ -453,3 +490,15 @@ 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) {
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(SubStr) != std::string::npos) {};
}