Reland "[clang-tidy] fix bugprone-narrowing-conversions false positive for conditional expression" #151874
+61
−4
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
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Andrey (AndreyG) ChangesThis is another attempt to merge previously reverted PR #139474. The added tests Full diff: https://github.com/llvm/llvm-project/pull/151874.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index 53782231b6421..249c77ca0c432 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -555,15 +555,22 @@ bool NarrowingConversionsCheck::handleConditionalOperator(
// We have an expression like so: `output = cond ? lhs : rhs`
// From the point of view of narrowing conversion we treat it as two
// expressions `output = lhs` and `output = rhs`.
- handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
- *CO->getLHS());
- handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), Lhs,
- *CO->getRHS());
+ handleConditionalOperatorArgument(Context, Lhs, CO->getLHS());
+ handleConditionalOperatorArgument(Context, Lhs, CO->getRHS());
return true;
}
return false;
}
+void NarrowingConversionsCheck::handleConditionalOperatorArgument(
+ const ASTContext &Context, const Expr &Lhs, const Expr *Arg) {
+ if (const auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(Arg))
+ if (!Arg->getIntegerConstantExpr(Context))
+ Arg = ICE->getSubExpr();
+
+ handleBinaryOperator(Context, Arg->getExprLoc(), Lhs, *Arg);
+}
+
void NarrowingConversionsCheck::handleImplicitCast(
const ASTContext &Context, const ImplicitCastExpr &Cast) {
if (Cast.getExprLoc().isMacroID())
diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
index 20403f920b925..116a8cba8d321 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
@@ -85,6 +85,8 @@ class NarrowingConversionsCheck : public ClangTidyCheck {
bool handleConditionalOperator(const ASTContext &Context, const Expr &Lhs,
const Expr &Rhs);
+ void handleConditionalOperatorArgument(const ASTContext &Context,
+ const Expr &Lhs, const Expr *Arg);
void handleImplicitCast(const ASTContext &Context,
const ImplicitCastExpr &Cast);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 85b31bc0b42a6..004130c9472b0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -134,6 +134,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for
variables introduced by structured bindings.
+- Improved :doc:`bugprone-narrowing-conversions
+ <clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing
+ false positive from analysis of a conditional expression in C.
+
- Improved :doc:`bugprone-reserved-identifier
<clang-tidy/checks/bugprone/reserved-identifier>` check by ignoring
declarations in system headers.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c
new file mode 100644
index 0000000000000..5cb43744d560d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
+// RUN: -- -target x86_64-unknown-linux
+
+char test_char(int cond, char c) {
+ char ret = cond > 0 ? ':' : c;
+ return ret;
+}
+
+short test_short(int cond, short s) {
+ short ret = cond > 0 ? ':' : s;
+ return ret;
+}
+
+int test_int(int cond, int i) {
+ int ret = cond > 0 ? ':' : i;
+ return ret;
+}
+
+void test(int cond, int i) {
+ char x = cond > 0 ? ':' : i;
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp
new file mode 100644
index 0000000000000..5cb43744d560d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
+// RUN: -- -target x86_64-unknown-linux
+
+char test_char(int cond, char c) {
+ char ret = cond > 0 ? ':' : c;
+ return ret;
+}
+
+short test_short(int cond, short s) {
+ short ret = cond > 0 ? ':' : s;
+ return ret;
+}
+
+int test_int(int cond, int i) {
+ int ret = cond > 0 ? ':' : i;
+ return ret;
+}
+
+void test(int cond, int i) {
+ char x = cond > 0 ? ':' : i;
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]
+}
|
AndreyG
changed the title
…conditional expression (llvm#139474) Let's consider the following code from the issue llvm#139467: ```c void test(int cond, char c) { char ret = cond > 0 ? ':' : c; } ``` Initializer of `ret` looks the following: ``` -ImplicitCastExpr 'char' <IntegralCast> `-ConditionalOperator 'int' |-BinaryOperator 'int' '>' | |-ImplicitCastExpr 'int' <LValueToRValue> | | `-DeclRefExpr 'int' lvalue ParmVar 'cond' 'int' | `-IntegerLiteral 'int' 0 |-CharacterLiteral 'int' 58 `-ImplicitCastExpr 'int' <IntegralCast> `-ImplicitCastExpr 'char' <LValueToRValue> `-DeclRefExpr 'char' lvalue ParmVar 'c' 'char' ``` So it could be seen that `RHS` of the conditional operator is `DeclRefExpr 'c'` which is casted to `int` and then the whole conditional expression is casted to 'char'. But this last conversion is not narrowing, because `RHS` was `char` _initially_. We should just remove the cast from `char` to `int` before the narrowing conversion check. Fixes llvm#139467 The added tests contains the implementation-defined warning about 'int' to 'char' conversion, which is not applicable to all platforms. And so the target is explictly set to 'x86_64' (the line 'RUN: -- -target x86_64-unknown-linux').
vbvictor
changed the title
vbvictor
approved these changes
Aug 5, 2025
|
@vbvictor could you, please, merge the PR? |
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 is another attempt to merge previously reverted PR #139474. The added tests
narrowing-conversions-conditional-expressions.c[pp]failed on different (non x86_64) platforms because the expected warning is implementation-defined. That's why the test must explicitly specify target (the line// RUN: -- -target x86_64-unknown-linux).