[clang-tidy] rewrite matchers in modernize-use-starts-ends-with #112101
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
Rewrite the AST matchers for slightly more composability and performance. Furthermore, check that the `starts_with` and `ends_with` functions return a `bool`. There is one behavioral change, in that the methods of a class (and transitive classes) are searched once for a matching `starts_with`/`ends_with` function, picking the first it can find. Previously, the matchers would try to find `starts_with`, then `startsWith`, and finally, `startswith`. Now, the first of the three that is encountered will be the matched method.
|
@llvm/pr-subscribers-clang-tools-extra Author: Julian Schmidt (5chmidti) ChangesRewrite the AST matchers for slightly more composability and Full diff: https://github.com/llvm/llvm-project/pull/112101.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 5eb3267adb0799..a1376aa8fa5e6e 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -9,8 +9,10 @@
#include "UseStartsEndsWithCheck.h"
#include "../utils/ASTUtils.h"
-#include "../utils/OptionsUtils.h"
+#include "../utils/Matchers.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
#include <string>
@@ -82,34 +84,25 @@ UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto ZeroLiteral = integerLiteral(equals(0));
- const auto HasStartsWithMethodWithName = [](const std::string &Name) {
- return hasMethod(
- cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
- .bind("starts_with_fun"));
- };
- const auto HasStartsWithMethod =
- anyOf(HasStartsWithMethodWithName("starts_with"),
- HasStartsWithMethodWithName("startsWith"),
- HasStartsWithMethodWithName("startswith"));
- const auto OnClassWithStartsWithFunction =
- on(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
- anyOf(HasStartsWithMethod,
- hasAnyBase(hasType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(HasStartsWithMethod)))))))))));
-
- const auto HasEndsWithMethodWithName = [](const std::string &Name) {
- return hasMethod(
- cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
- .bind("ends_with_fun"));
- };
- const auto HasEndsWithMethod = anyOf(HasEndsWithMethodWithName("ends_with"),
- HasEndsWithMethodWithName("endsWith"),
- HasEndsWithMethodWithName("endswith"));
+ const auto ClassTypeWithMethod =
+ [](const StringRef MethodBoundName,
+ const llvm::ArrayRef<StringRef> &Methods) {
+ const auto Method =
+ cxxMethodDecl(isConst(), parameterCountIs(1),
+ returns(booleanType()), hasAnyName(Methods))
+ .bind(MethodBoundName);
+ return qualType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(hasMethod(Method),
+ hasAnyBase(hasType(hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(hasMethod(Method)))))))))));
+ };
+
+ const auto OnClassWithStartsWithFunction = on(hasType(ClassTypeWithMethod(
+ "starts_with_fun", {"starts_with", "startsWith", "startswith"})));
+
const auto OnClassWithEndsWithFunction =
- on(expr(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
- anyOf(HasEndsWithMethod,
- hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
- cxxRecordDecl(HasEndsWithMethod)))))))))))
+ on(expr(hasType(ClassTypeWithMethod(
+ "ends_with_fun", {"ends_with", "endsWith", "endswith"})))
.bind("haystack"));
// Case 1: X.find(Y) [!=]= 0 -> starts_with.
@@ -145,7 +138,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// All cases comparing to 0.
Finder->addMatcher(
binaryOperator(
- hasAnyOperatorName("==", "!="),
+ matchers::isEqualityOperator(),
hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr,
CompareEndsWithExpr))
.bind("find_expr"),
@@ -156,7 +149,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// Case 5: X.rfind(Y) [!=]= LEN(X) - LEN(Y) -> ends_with.
Finder->addMatcher(
binaryOperator(
- hasAnyOperatorName("==", "!="),
+ matchers::isEqualityOperator(),
hasOperands(
cxxMemberCallExpr(
anyOf(
@@ -190,9 +183,8 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
const CXXMethodDecl *ReplacementFunction =
StartsWithFunction ? StartsWithFunction : EndsWithFunction;
- if (ComparisonExpr->getBeginLoc().isMacroID()) {
+ if (ComparisonExpr->getBeginLoc().isMacroID())
return;
- }
const bool Neg = ComparisonExpr->getOpcode() == BO_NE;
@@ -220,9 +212,8 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
(ReplacementFunction->getName() + "(").str());
// Add possible negation '!'.
- if (Neg) {
+ if (Neg)
Diagnostic << FixItHint::CreateInsertion(FindExpr->getBeginLoc(), "!");
- }
}
} // namespace clang::tidy::modernize
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 798af260a3b66c..9365aeac068ee2 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
@@ -150,8 +150,8 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-FIXES: puv.starts_with("a");
puvf.find("a") == 0;
- // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
- // CHECK-FIXES: puvf.starts_with("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use startsWith
+ // CHECK-FIXES: puvf.startsWith("a");
// Here, the subclass has startsWith, the superclass has starts_with.
// We prefer the version from the subclass.
|
|
@llvm/pr-subscribers-clang-tidy Author: Julian Schmidt (5chmidti) ChangesRewrite the AST matchers for slightly more composability and Full diff: https://github.com/llvm/llvm-project/pull/112101.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 5eb3267adb0799..a1376aa8fa5e6e 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -9,8 +9,10 @@
#include "UseStartsEndsWithCheck.h"
#include "../utils/ASTUtils.h"
-#include "../utils/OptionsUtils.h"
+#include "../utils/Matchers.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
#include <string>
@@ -82,34 +84,25 @@ UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto ZeroLiteral = integerLiteral(equals(0));
- const auto HasStartsWithMethodWithName = [](const std::string &Name) {
- return hasMethod(
- cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
- .bind("starts_with_fun"));
- };
- const auto HasStartsWithMethod =
- anyOf(HasStartsWithMethodWithName("starts_with"),
- HasStartsWithMethodWithName("startsWith"),
- HasStartsWithMethodWithName("startswith"));
- const auto OnClassWithStartsWithFunction =
- on(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
- anyOf(HasStartsWithMethod,
- hasAnyBase(hasType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(HasStartsWithMethod)))))))))));
-
- const auto HasEndsWithMethodWithName = [](const std::string &Name) {
- return hasMethod(
- cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
- .bind("ends_with_fun"));
- };
- const auto HasEndsWithMethod = anyOf(HasEndsWithMethodWithName("ends_with"),
- HasEndsWithMethodWithName("endsWith"),
- HasEndsWithMethodWithName("endswith"));
+ const auto ClassTypeWithMethod =
+ [](const StringRef MethodBoundName,
+ const llvm::ArrayRef<StringRef> &Methods) {
+ const auto Method =
+ cxxMethodDecl(isConst(), parameterCountIs(1),
+ returns(booleanType()), hasAnyName(Methods))
+ .bind(MethodBoundName);
+ return qualType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(hasMethod(Method),
+ hasAnyBase(hasType(hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(hasMethod(Method)))))))))));
+ };
+
+ const auto OnClassWithStartsWithFunction = on(hasType(ClassTypeWithMethod(
+ "starts_with_fun", {"starts_with", "startsWith", "startswith"})));
+
const auto OnClassWithEndsWithFunction =
- on(expr(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
- anyOf(HasEndsWithMethod,
- hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
- cxxRecordDecl(HasEndsWithMethod)))))))))))
+ on(expr(hasType(ClassTypeWithMethod(
+ "ends_with_fun", {"ends_with", "endsWith", "endswith"})))
.bind("haystack"));
// Case 1: X.find(Y) [!=]= 0 -> starts_with.
@@ -145,7 +138,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// All cases comparing to 0.
Finder->addMatcher(
binaryOperator(
- hasAnyOperatorName("==", "!="),
+ matchers::isEqualityOperator(),
hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr,
CompareEndsWithExpr))
.bind("find_expr"),
@@ -156,7 +149,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
// Case 5: X.rfind(Y) [!=]= LEN(X) - LEN(Y) -> ends_with.
Finder->addMatcher(
binaryOperator(
- hasAnyOperatorName("==", "!="),
+ matchers::isEqualityOperator(),
hasOperands(
cxxMemberCallExpr(
anyOf(
@@ -190,9 +183,8 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
const CXXMethodDecl *ReplacementFunction =
StartsWithFunction ? StartsWithFunction : EndsWithFunction;
- if (ComparisonExpr->getBeginLoc().isMacroID()) {
+ if (ComparisonExpr->getBeginLoc().isMacroID())
return;
- }
const bool Neg = ComparisonExpr->getOpcode() == BO_NE;
@@ -220,9 +212,8 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
(ReplacementFunction->getName() + "(").str());
// Add possible negation '!'.
- if (Neg) {
+ if (Neg)
Diagnostic << FixItHint::CreateInsertion(FindExpr->getBeginLoc(), "!");
- }
}
} // namespace clang::tidy::modernize
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 798af260a3b66c..9365aeac068ee2 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
@@ -150,8 +150,8 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-FIXES: puv.starts_with("a");
puvf.find("a") == 0;
- // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
- // CHECK-FIXES: puvf.starts_with("a");
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use startsWith
+ // CHECK-FIXES: puvf.startsWith("a");
// Here, the subclass has startsWith, the superclass has starts_with.
// We prefer the version from the subclass.
|
|
Note that his is not an NFC due to the change in what method may be found first, but given that it's quite out there to have 2+ of |
5chmidti
requested review from
PiotrZSL,
nicovank,
EugeneZelenko,
SimplyDanny and
HerrCai0907
nicovank
reviewed
Oct 12, 2024
clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
nicovank
approved these changes
Oct 13, 2024
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Nicolas van Kempen <[email protected]>
5chmidti
commented
Oct 13, 2024
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
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.
Rewrite the AST matchers for slightly more composability.
Furthermore, check that the
starts_withandends_withfunctions return a
bool.There is one behavioral change, in that the methods of a class (and
transitive classes) are searched once for a matching
starts_with/ends_withfunction, picking the first it can find.Previously, the matchers would try to find
starts_with, thenstartsWith, and finally,startswith. Now, the first of the three thatis encountered will be the matched method.