[clang-tidy] 'modernize-use-starts-ends-with': fixed false positives on find and rfind
#129564
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Baranov Victor (vbvictor) Changes
Closes #129498. P.S. In the issue one of the proposed fix to the problem was to transform I can implement creation of Full diff: https://github.com/llvm/llvm-project/pull/129564.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index ab72c6796bb4c..02a537cccc430 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -113,9 +113,10 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto OnClassWithEndsWithFunction = ClassTypeWithMethod(
"ends_with_fun", "ends_with", "endsWith", "endswith", "EndsWith");
- // Case 1: X.find(Y) [!=]= 0 -> starts_with.
+ // Case 1: X.find(Y, [0]) [!=]= 0 -> starts_with.
const auto FindExpr = cxxMemberCallExpr(
- anyOf(argumentCountIs(1), hasArgument(1, ZeroLiteral)),
+ anyOf(argumentCountIs(1),
+ allOf(argumentCountIs(2), hasArgument(1, ZeroLiteral))),
callee(
cxxMethodDecl(hasName("find"), ofClass(OnClassWithStartsWithFunction))
.bind("find_fun")),
@@ -123,7 +124,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// Case 2: X.rfind(Y, 0) [!=]= 0 -> starts_with.
const auto RFindExpr = cxxMemberCallExpr(
- hasArgument(1, ZeroLiteral),
+ argumentCountIs(2), hasArgument(1, ZeroLiteral),
callee(cxxMethodDecl(hasName("rfind"),
ofClass(OnClassWithStartsWithFunction))
.bind("find_fun")),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 07a79d6bbe807..e29598ccf0b43 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -137,6 +137,10 @@ Changes in existing checks
<clang-tidy/checks/performance/move-const-arg>` check by fixing false negatives
on ternary operators calling ``std::move``.
+- Improved :doc:`modernize-use-starts-ends-with
+ <clang-tidy/checks/modernize/use-starts-ends-with>` check by fixing false
+ positives on methods ``find`` and ``rfind`` called with three arguments.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
index 78cd900885ac3..bad1952db22f9 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
@@ -13,6 +13,7 @@ Covered scenarios:
Expression Replacement
---------------------------------------------------- ---------------------
``u.find(v) == 0`` ``u.starts_with(v)``
+``u.find(v, 0) == 0`` ``u.starts_with(v)``
``u.rfind(v, 0) != 0`` ``!u.starts_with(v)``
``u.compare(0, v.size(), v) == 0`` ``u.starts_with(v)``
``u.substr(0, v.size()) == v`` ``u.starts_with(v)``
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
index 8699ca03ba331..4d61709eff463 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
@@ -236,6 +236,18 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
// CHECK-FIXES: s.ends_with(suffix);
+ s.find("a", 0) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with("a");
+
+ s.find(s, ZERO) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ s.find(s, 0) == ZERO;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
struct S {
std::string s;
} t;
@@ -261,6 +273,9 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
#define STRING s
if (0 == STRING.find("ala")) { /* do something */}
+
+ s.find("aaa", 0, 3) == 0;
+ s.rfind("aaa", std::string::npos, 3) == 0;
}
void test_substr() {
|
|
@llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) Changes
Closes #129498. P.S. In the issue one of the proposed fix to the problem was to transform I can implement creation of Full diff: https://github.com/llvm/llvm-project/pull/129564.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index ab72c6796bb4c..02a537cccc430 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -113,9 +113,10 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto OnClassWithEndsWithFunction = ClassTypeWithMethod(
"ends_with_fun", "ends_with", "endsWith", "endswith", "EndsWith");
- // Case 1: X.find(Y) [!=]= 0 -> starts_with.
+ // Case 1: X.find(Y, [0]) [!=]= 0 -> starts_with.
const auto FindExpr = cxxMemberCallExpr(
- anyOf(argumentCountIs(1), hasArgument(1, ZeroLiteral)),
+ anyOf(argumentCountIs(1),
+ allOf(argumentCountIs(2), hasArgument(1, ZeroLiteral))),
callee(
cxxMethodDecl(hasName("find"), ofClass(OnClassWithStartsWithFunction))
.bind("find_fun")),
@@ -123,7 +124,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// Case 2: X.rfind(Y, 0) [!=]= 0 -> starts_with.
const auto RFindExpr = cxxMemberCallExpr(
- hasArgument(1, ZeroLiteral),
+ argumentCountIs(2), hasArgument(1, ZeroLiteral),
callee(cxxMethodDecl(hasName("rfind"),
ofClass(OnClassWithStartsWithFunction))
.bind("find_fun")),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 07a79d6bbe807..e29598ccf0b43 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -137,6 +137,10 @@ Changes in existing checks
<clang-tidy/checks/performance/move-const-arg>` check by fixing false negatives
on ternary operators calling ``std::move``.
+- Improved :doc:`modernize-use-starts-ends-with
+ <clang-tidy/checks/modernize/use-starts-ends-with>` check by fixing false
+ positives on methods ``find`` and ``rfind`` called with three arguments.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
index 78cd900885ac3..bad1952db22f9 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
@@ -13,6 +13,7 @@ Covered scenarios:
Expression Replacement
---------------------------------------------------- ---------------------
``u.find(v) == 0`` ``u.starts_with(v)``
+``u.find(v, 0) == 0`` ``u.starts_with(v)``
``u.rfind(v, 0) != 0`` ``!u.starts_with(v)``
``u.compare(0, v.size(), v) == 0`` ``u.starts_with(v)``
``u.substr(0, v.size()) == v`` ``u.starts_with(v)``
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
index 8699ca03ba331..4d61709eff463 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
@@ -236,6 +236,18 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
// CHECK-FIXES: s.ends_with(suffix);
+ s.find("a", 0) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with("a");
+
+ s.find(s, ZERO) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ s.find(s, 0) == ZERO;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
struct S {
std::string s;
} t;
@@ -261,6 +273,9 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
#define STRING s
if (0 == STRING.find("ala")) { /* do something */}
+
+ s.find("aaa", 0, 3) == 0;
+ s.rfind("aaa", std::string::npos, 3) == 0;
}
void test_substr() {
|
nicovank
reviewed
Mar 3, 2025
There was a problem hiding this comment.
Thanks!! I think something could also be done incorporating lengthExprForStringNode, with this patch some TPs will be ignored. lengthExprForStringNode will ignore char* unless strlen is used in which case the string has to be null-terminated anyway. What do you think? EDIT: or a char* literal, in which case it's also null terminated.
I'm also okay with landing this ignore now, those FPs are probably exceedingly rare.
Creating string views is more involved and I also think not needed for this change.
clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
Outdated
Show resolved
Hide resolved
|
Reverted style changes in |
nicovank
approved these changes
Mar 13, 2025
|
Since commit access request takes time than I expected, could you please merge this PR, @nicovank? |
nicovank
reviewed
Mar 21, 2025
|
Actually, with the other one merged I feel like there'll be a conflict with GitHub trying to auto-merge. Can you rebase on main? Then I'll click the button. |
Please make sure that Release Notes entries is in alphabetical order after rebase. There should be merge conflicts because of changes in Release Notes. |
vbvictor
force-pushed
the
modernize-use-starts-ends-with-issue129498
branch
from
50c73aa to
e72ae0e
Compare
|
Rebase and PR-checks are done, ready to merge |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
s.rfind(v, 0, 3)as if it had only 2 arguments.u.find(v, 0) == 0case.Closes #129498.
P.S. In the issue one of the proposed fix to the problem was to transform
s.find(v, 0, 3)intos.starts_with({v, 3}). But for me creating astring_view"on the go" is confusing since we create another object, what do others think?FYI @nicovank
I can implement creation of
string_viewbut for now I ignore those cases.