Skip to content

Revert "[clang-tidy] fix bugprone-narrowing-conversions false positive for conditional expression" #151859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 3, 2025

Conversation

vbvictor
Copy link
Contributor

@vbvictor vbvictor commented Aug 3, 2025

Reverts #139474 due to lit test failures on arm platforms.

@llvmbot
Copy link
Member

llvmbot commented Aug 3, 2025

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

Changes

Reverts llvm/llvm-project#139474 due to lit failures on arm platforms.


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

5 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp (+4-11)
  • (modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h (-2)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (-4)
  • (removed) clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c (-21)
  • (removed) clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp (-21)
diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index 249c77ca0c432..53782231b6421 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -555,22 +555,15 @@ 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`.
-    handleConditionalOperatorArgument(Context, Lhs, CO->getLHS());
-    handleConditionalOperatorArgument(Context, Lhs, CO->getRHS());
+    handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
+                         *CO->getLHS());
+    handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), 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 116a8cba8d321..20403f920b925 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
@@ -85,8 +85,6 @@ 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 b9261f61c8789..ef0e804962927 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -134,10 +134,6 @@ 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
deleted file mode 100644
index 0ea83bc8559af..0000000000000
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
-
-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
deleted file mode 100644
index 0ea83bc8559af..0000000000000
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
-
-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]
-}

@llvmbot
Copy link
Member

llvmbot commented Aug 3, 2025

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

Author: Baranov Victor (vbvictor)

Changes

Reverts llvm/llvm-project#139474 due to lit failures on arm platforms.


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

5 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp (+4-11)
  • (modified) clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h (-2)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (-4)
  • (removed) clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c (-21)
  • (removed) clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp (-21)
diff --git a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
index 249c77ca0c432..53782231b6421 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp
@@ -555,22 +555,15 @@ 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`.
-    handleConditionalOperatorArgument(Context, Lhs, CO->getLHS());
-    handleConditionalOperatorArgument(Context, Lhs, CO->getRHS());
+    handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
+                         *CO->getLHS());
+    handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), 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 116a8cba8d321..20403f920b925 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h
@@ -85,8 +85,6 @@ 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 b9261f61c8789..ef0e804962927 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -134,10 +134,6 @@ 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
deleted file mode 100644
index 0ea83bc8559af..0000000000000
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.c
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
-
-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
deleted file mode 100644
index 0ea83bc8559af..0000000000000
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/narrowing-conversions-conditional-expressions.cpp
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %check_clang_tidy %s bugprone-narrowing-conversions %t -- --
-
-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]
-}

@vbvictor vbvictor merged commit e44a14e into main Aug 3, 2025
11 of 13 checks passed
@vbvictor vbvictor deleted the revert-139474-139467 branch August 3, 2025 11:00
krishna2803 pushed a commit to krishna2803/llvm-project that referenced this pull request Aug 12, 2025
…ive for conditional expression" (llvm#151859)

Reverts llvm#139474 due to lit test failures on `arm`
platforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants