[Clang] Separate implicit int conversion on negation sign to new diagnostic group #139429
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
YutongZhuu
force-pushed
the
dev_issue_18878
branch
from
249f4ba to
b3b1714
Compare
YutongZhuu
changed the title
-Wimplicit-int-conversion introduced in #126846
YutongZhuu
changed the title
-Wimplicit-int-conversion introduced in #126846-Wimplicit-int-conversionwarning introduced in #126846
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Yutong Zhu (YutongZhuu) ChangesThis PR reverts a change made in #126846. #126846 introduced an int8_t x = something;
x = -x; // warning hereThis 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:
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
|
|
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 |
|
I think it makes sense to split this off into its own warning group so it can be controlled separately from |
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 |
YutongZhuu
force-pushed
the
dev_issue_18878
branch
from
b3b1714 to
e9e4634
Compare
YutongZhuu
changed the title
-Wimplicit-int-conversionwarning introduced in #126846
H
Hi, could you check if this is what you suggested? |
cor3ntin
reviewed
May 16, 2025
clang/lib/Sema/SemaChecking.cpp
Outdated
Comment on lines
12094
to
12099
| if (const auto *UO = dyn_cast<UnaryOperator>(E)) { | ||
| return DiagnoseImpCast(*this, E, T, CC, | ||
| diag::warn_impcast_integer_precision_on_negation); | ||
| } | ||
|
|
There was a problem hiding this comment.
We should check which operator it is. Did you try to add tests with
unsigned char test_diag(unsigned char x) {
return +x;
}```
There was a problem hiding this comment.
Sorry, that was a mistake.
Comment on lines
1
to
37
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion | ||
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion -Wno-implicit-int-conversion-on-negation -DNO_DIAG | ||
|
|
||
| #ifdef NO_DIAG | ||
| unsigned char test_no_diag(unsigned char x) { | ||
| return -x; // expected-no-diagnostics | ||
| } | ||
| #else | ||
| unsigned char test_diag(unsigned char x) { | ||
| return -x; // expected-warning {{implicit conversion loses integer precision: 'int' to 'unsigned char' on negation}} | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Suggested change
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion | |
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion -Wno-implicit-int-conversion-on-negation -DNO_DIAG | |
| #ifdef NO_DIAG | |
| unsigned char test_no_diag(unsigned char x) { | |
| return -x; // expected-no-diagnostics | |
| } | |
| #else | |
| unsigned char test_diag(unsigned char x) { | |
| return -x; // expected-warning {{implicit conversion loses integer precision: 'int' to 'unsigned char' on negation}} | |
| } | |
| #endif | |
| // RUN: %clang_cc1 %s -verify=nowarn -Wimplicit-int-conversion | |
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion -Wno-implicit-int-conversion-on-negation -DNO_DIAG | |
| // nowarn-no-diagnostics | |
| unsigned char test_no_diag(unsigned char x) { | |
| return -x; // expected-warning {{implicit conversion loses integer precision: 'int' to 'unsigned char' on negation}} | |
| } |
There was a problem hiding this comment.
I wasn't able to get this to work, if you are worried about code duplication, is the current version better?
YutongZhuu
force-pushed
the
dev_issue_18878
branch
from
87247a3 to
b5a1833
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
YutongZhuu
force-pushed
the
dev_issue_18878
branch
from
b5a1833 to
0d2dcde
Compare
|
@cor3ntin Hi, sorry to bother, I think this is ready for review :) |
clang/docs/ReleaseNotes.rst
Outdated
Comment on lines
355
to
356
| - Split diagnosis of implicit integer comparison on negation to a new diagnostic group ``-Wimplicit-int-comparison-on-negation``, | ||
| so user can turn it off independently. |
There was a problem hiding this comment.
Suggested change
| - Split diagnosis of implicit integer comparison on negation to a new diagnostic group ``-Wimplicit-int-comparison-on-negation``, | |
| so user can turn it off independently. | |
| - Split diagnosis of implicit integer comparison on negation to a new | |
| diagnostic group ``-Wimplicit-int-comparison-on-negation``, grouped under | |
| ``-Wimplicit-int-conversion``, so user can turn it off independently. |
| @@ -0,0 +1,64 @@ | |||
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion | |||
| // RUN: %clang_cc1 %s -verify -Wimplicit-int-conversion -Wno-implicit-int-conversion-on-negation -DNO_DIAG | |||
There was a problem hiding this comment.
A better approach than using -D is to specify a prefix to -verify: https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics
Then you can have one comment for the -no-diagnostics RUN line and skip all the preprocessor code for the other RUN line.
| char test_char(char x) { | ||
| return -x; | ||
| #ifndef NO_DIAG | ||
| // expected-warning@-2 {{implicit conversion loses integer precision}} |
There was a problem hiding this comment.
Is this actually checking for the new diagnostic ("on negation")? If so, you should update the expected warning to include the whole diagnostic text.
Comment on lines
+18
to
+20
| unsigned short test_unsigned_short(unsigned short x) { | ||
| return -x; // expected-warning {{implicit conversion loses integer precision: 'int' to 'unsigned short' on negation}} | ||
| } |
There was a problem hiding this comment.
One last test and I think we're good:
unsigned _BitInt(16) test_unsigned_bit_int(unsigned _BitInt(16) x) {
return -x;
}
this should not diagnose because _BitInt does not undergo promotion.
AaronBallman
approved these changes
May 30, 2025
Hi, I don't have commit access, so I might need you to merge it for me :) |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/10928 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/9873 Here is the relevant piece of the build log for the reference |
| @@ -352,6 +352,10 @@ Improvements to Clang's diagnostics | |||
| - Now correctly diagnose a tentative definition of an array with static | |||
| storage duration in pedantic mode in C. (#GH50661) | |||
|
|
|||
| - Split diagnosis of implicit integer comparison on negation to a new | |||
| diagnostic group ``-Wimplicit-int-comparison-on-negation``, grouped under | |||
There was a problem hiding this comment.
typo here: it spells -Wimplicit-int-conversion-on-negation in the actual implementation
DhruvSrivastavaX
pushed a commit
to DhruvSrivastavaX/lldb-for-aix
that referenced
this pull request
Jun 12, 2025
…nostic group (llvm#139429) This PR reverts a change made in llvm#126846. llvm#126846 introduced an ``-Wimplicit-int-conversion`` diagnosis for ```c++ 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 llvm#126846: ```c++ 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.
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.
This PR reverts a change made in #126846.
#126846 introduced an
-Wimplicit-int-conversiondiagnosis forThis 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:
This false positive is uncommon, so I think it makes sense to revert the change.