[clang] Improve -Wsign-compare diagnostic

[halbi2]

The cv-qualifiers are not relevant to the comparison result so one should not print them.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (halbi2)

Changes

The cv-qualifiers are not relevant to the comparison result so one should not print them.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+2-1)
• (modified) clang/test/SemaCXX/compare.cpp (+8-8)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 74f425d32648f..5c664eebeb0c5 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -10593,7 +10593,8 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
 
   S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
                         S.PDiag(diag::warn_mixed_sign_comparison)
-                            << LHS->getType() << RHS->getType()
+                            << LHS->getType().getUnqualifiedType()
+                            << RHS->getType().getUnqualifiedType()
                             << LHS->getSourceRange() << RHS->getSourceRange());
 }
 
diff --git a/clang/test/SemaCXX/compare.cpp b/clang/test/SemaCXX/compare.cpp
index cfddf2142f308..d678adc38efd1 100644
--- a/clang/test/SemaCXX/compare.cpp
+++ b/clang/test/SemaCXX/compare.cpp
@@ -233,10 +233,10 @@ void test4(short s) {
   // All negative shorts are cast towards the max unsigned range.  Relation
   // comparisons are possible, but equality comparisons are tautological.
   const unsigned A = 32768;
-  void (s < A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
-  void (s > A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
-  void (s <= A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
-  void (s >= A); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
+  void (s < A); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
+  void (s > A); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
+  void (s <= A); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
+  void (s >= A); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
 
   void (s == A); // expected-warning{{comparison of constant 32768 with expression of type 'short' is always false}}
   void (s != A); // expected-warning{{comparison of constant 32768 with expression of type 'short' is always true}}
@@ -245,12 +245,12 @@ void test4(short s) {
   // unsigned.  Likewise, a negative one short can also be converted to max
   // unsigned.
   const unsigned B = -1;
-  void (s < B); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
+  void (s < B); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
   void (s > B); // expected-warning{{comparison 'short' > 4294967295 is always false}}
   void (s <= B); // expected-warning{{comparison 'short' <= 4294967295 is always true}}
-  void (s >= B); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
-  void (s == B); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
-  void (s != B); // expected-warning{{comparison of integers of different signs: 'short' and 'const unsigned int'}}
+  void (s >= B); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
+  void (s == B); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
+  void (s != B); // expected-warning{{comparison of integers of different signs: 'short' and 'unsigned int'}}
 
 }
 

[Fznamznon]

Thank you for the patch! I think it is a minor wording change, so it doesn't need a changelog.
However, I think this is technically not the same as #82109 . There we also say that we are initializing a constant. Here we technically lose the info about const-ness in the output message which is irrelevant for signs in comparison, still maybe good for the user to see clearly what type there is. I think clang is not wrong here saying that the type is const. So, I'm adding more reviewers for opinions.

[erichkeane]

I don't think this patch is the right way forward. We shouldn't lie to the user about the type, and have to count on the left side of the diagnostic to tell them how to analyze the types to figure out the problem. Qualifiers are part of the type, so being incorrect here isn't something we typically do.