[clang-tidy] Fix a broken fix-it provided by modernize-use-integer-sign-comparison

[localspook]

Fixes #162981.

This is an issue with both named and functional casts, but this check doesn't have any existing tests for the latter, so I went and added those.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Victor Chernyakin (localspook)

Changes

Fixes #162981.

This is an issue with both named and functional casts, but this check doesn't actually have tests for the latter, so I went and added those.

---

Full diff: https://github.com/llvm/llvm-project/pull/163488.diff

4 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+2)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison-qt.cpp (+6-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp (+6-2)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
index 0003429c62890..77262ebdcf772 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
@@ -152,6 +152,8 @@ void UseIntegerSignComparisonCheck::check(
   if (const auto *RHSCast = llvm::dyn_cast<ExplicitCastExpr>(RHS)) {
     SubExprRHS = RHSCast->getSubExpr();
     R2.setEnd(SubExprRHS->getBeginLoc().getLocWithOffset(-1));
+    R3.setBegin(Lexer::getLocForEndOfToken(
+        SubExprRHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts()));
   }
   DiagnosticBuilder Diag =
       diag(BinaryOp->getBeginLoc(),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 33cc401bcb78f..59be94ae36933 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -359,6 +359,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-designated-initializers>` check to
   suggest using designated initializers for aliased aggregate types.
 
+- Improved :doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` by providing
+  correct fix-its when the RHS of a comparison contains a non-C-style cast.
+
 - Improved :doc:`modernize-use-nullptr
   <clang-tidy/checks/modernize/use-nullptr>` check by fixing a crash
   on Windows when the check was enabled with a 32-bit :program:`clang-tidy`
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison-qt.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison-qt.cpp
index 1f26ff34a4d04..31a3677c2bbd0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison-qt.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison-qt.cpp
@@ -92,8 +92,7 @@ int AllComparisons() {
     if (static_cast<unsigned int>(uArray[2]) < static_cast<int>(sArray[2]))
         return 0;
 // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
-// CHECK-FIXES: if (q20::cmp_less(uArray[2],sArray[2])))
-// FIXME: There should only be 2 closing braces. The fix-it inserts an unbalanced one.
+// CHECK-FIXES: if (q20::cmp_less(uArray[2],sArray[2]))
 
     if ((unsigned int)uArray[3] < (int)sArray[3])
         return 0;
@@ -116,6 +115,11 @@ int AllComparisons() {
 // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
 // CHECK-FIXES: if (q20::cmp_greater(uArray[6] , VALUE))
 
+    if (unsigned(uArray[7]) >= int(sArray[7]))
+        return 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (q20::cmp_greater_equal(uArray[7],sArray[7]))
+
 
     FuncParameters(uVar);
     TemplateFuncParameter(sVar);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
index 628cee0bb0de7..e7981a6d41883 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
@@ -91,8 +91,7 @@ int AllComparisons() {
     if (static_cast<unsigned int>(uArray[2]) < static_cast<int>(sArray[2]))
         return 0;
 // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
-// CHECK-FIXES: if (std::cmp_less(uArray[2],sArray[2])))
-// FIXME: There should only be 2 closing braces. The fix-it inserts an unbalanced one.
+// CHECK-FIXES: if (std::cmp_less(uArray[2],sArray[2]))
 
     if ((unsigned int)uArray[3] < (int)sArray[3])
         return 0;
@@ -115,6 +114,11 @@ int AllComparisons() {
 // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
 // CHECK-FIXES: if (std::cmp_greater(uArray[6] , VALUE))
 
+    if (unsigned(uArray[7]) >= int(sArray[7]))
+        return 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_greater_equal(uArray[7],sArray[7]))
+
 
     FuncParameters(uVar);
     TemplateFuncParameter(sVar);

[localspook]

Notice how (before this change) this if statement and the one before it were asymmetric: the previous one correctly adjusted two ranges (R1 and R2), while this one incorrectly adjusted only one (R2).

[vbvictor]

Wish we had meaningful names for R1, R2, R3. Could be a later NFC patch

[vbvictor]

Wish we had meaningful names for R1, R2, R3. Could be a later NFC patch