[Clang] Suppress a -Wimplicit-int-conversionwarning introduced in #126846

[YutongZhuu]

This PR reverts a change made in #126846.

#126846 introduced an -Wimplicit-int-conversion diagnosis for

int8_t x = something;
x = -x; // warning here

This is technically correct since -x could have a width of 9, but this pattern is common in codebases.

Reverting this change would also introduce the false positive I fixed in #126846:

bool b(signed char c) {
  return -c >= 128; // -c can be 128
}

This false positive is uncommon, so I think it makes sense to revert the change.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Yutong Zhu (YutongZhuu)

Changes

This PR reverts a change made in #126846.

#126846 introduced an -Wimplicit-int-conversion diagnosis for

int8_t x = something;
x = -x; // warning here

This is technically correct since -x could have a width of 9, but this pattern is common in codebases.

Reverting this change would also introduce the false positive I fixed in #126846:

bool b(signed char c) {
  return -c >= 128; // -c can be 128
}

This false positive is uncommon, so I think it makes sense to revert the change.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/Sema/SemaChecking.cpp (+5-19)
• (modified) clang/test/Sema/compare.c (+3-20)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 11f62bc881b03..edbcbea7828b2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -352,6 +352,9 @@ Improvements to Clang's diagnostics
 - Now correctly diagnose a tentative definition of an array with static
   storage duration in pedantic mode in C. (#GH50661)
 
+- The -Wimplicit-int-conversion warning no longer triggers for direct assignments between integer types narrower than int. 
+  However, -Wsign-compare can now incorrectly produce a warning when comparing a value to another with just one more bit of width.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index bffd0dd461d3d..084c3dbdecb20 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -10626,25 +10626,7 @@ static std::optional<IntRange> TryGetExprRange(ASTContext &C, const Expr *E,
     case UO_AddrOf: // should be impossible
       return IntRange::forValueOfType(C, GetExprType(E));
 
-    case UO_Minus: {
-      if (E->getType()->isUnsignedIntegerType()) {
-        return TryGetExprRange(C, UO->getSubExpr(), MaxWidth, InConstantContext,
-                               Approximate);
-      }
-
-      std::optional<IntRange> SubRange = TryGetExprRange(
-          C, UO->getSubExpr(), MaxWidth, InConstantContext, Approximate);
-
-      if (!SubRange)
-        return std::nullopt;
-
-      // If the range was previously non-negative, we need an extra bit for the
-      // sign bit. If the range was not non-negative, we need an extra bit
-      // because the negation of the most-negative value is one bit wider than
-      // that value.
-      return IntRange(SubRange->Width + 1, false);
-    }
-
+    case UO_Minus:
     case UO_Not: {
       if (E->getType()->isUnsignedIntegerType()) {
         return TryGetExprRange(C, UO->getSubExpr(), MaxWidth, InConstantContext,
@@ -10659,6 +10641,10 @@ static std::optional<IntRange> TryGetExprRange(ASTContext &C, const Expr *E,
 
       // The width increments by 1 if the sub-expression cannot be negative
       // since it now can be.
+      // This isn't technically correct for UO_Minus since we need an extra bit
+      // because the negation of the most-negative value is one bit wider than
+      // the original value. However, the correct version triggers many unwanted
+      // warnings.
       return IntRange(SubRange->Width + (int)SubRange->NonNegative, false);
     }
 
diff --git a/clang/test/Sema/compare.c b/clang/test/Sema/compare.c
index fdae3bc19841e..f8c4694b8730e 100644
--- a/clang/test/Sema/compare.c
+++ b/clang/test/Sema/compare.c
@@ -454,16 +454,6 @@ int test20(int n) {
 }
 #endif
 
-int test21(short n) {
-  return -n == 32768; // no-warning
-}
-
-#if TEST == 1
-int test22(short n) {
-  return -n == 65536; // expected-warning {{result of comparison of 17-bit signed value == 65536 is always false}}
-}
-#endif
-
 int test23(unsigned short n) {
   return ~n == 32768; // no-warning
 }
@@ -471,13 +461,6 @@ int test23(unsigned short n) {
 int test24(short n) {
   return ~n == 32767; // no-warning
 }
-
-#if TEST == 1
-int test25(unsigned short n) {
-  return ~n == 65536; // expected-warning {{result of comparison of 17-bit signed value == 65536 is always false}}
-}
-
-int test26(short n) {
-  return ~n == 32768; // expected-warning {{result of comparison of 16-bit signed value == 32768 is always false}}
-}
-#endif
+unsigned char test25(unsigned char n) {
+  return -n; // no-warning
+}
\ No newline at end of file

[cor3ntin]

If that warning is disruptive, maybe we should consider making it a separate warning that people can disable independently? It is certainly a potential source of bugs, so I'm not sure removing it entirely is the right approach
@AaronBallman

[AaronBallman]

I think it makes sense to split this off into its own warning group so it can be controlled separately from -Wimplicit-int-conversion. It is a common code pattern, but it can still hide bugs that can be hard to spot when the values are on the edge of the range of representable values for the type.

[YutongZhuu]

I think it makes sense to split this off into its own warning group so it can be controlled separately from -Wimplicit-int-conversion. It is a common code pattern, but it can still hide bugs that can be hard to spot when the values are on the edge of the range of representable values for the type.

You mean we should keep my previous changes and separate this pattern

int8_t x = something; 
x = -x;

to a new warning flag?

[AaronBallman]

I think it makes sense to split this off into its own warning group so it can be controlled separately from -Wimplicit-int-conversion. It is a common code pattern, but it can still hide bugs that can be hard to spot when the values are on the edge of the range of representable values for the type.

You mean we should keep my previous changes and separate this pattern

int8_t x = something; 
x = -x;

to a new warning flag?

Correct. Perhaps something like -Wimplicit-int-conversion-on-negation which is grouped under -Wimplicit-int-conversion?