[llvm] [clang] [clang-tools-extra] [clang-tidy] Fixes to readability-implicit-bool-conversion (PR #72050)

[Piotr Zegar via cfe-commits]

https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/72050

>From 09bc9ecebe2e9d49a690c9864aa09d5b8c4403e4 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Sun, 12 Nov 2023 11:31:21 +0000
Subject: [PATCH 1/3] [clang-tidy] Fixes to
 readability-implicit-bool-conversion

- Fixed issue with invalid code being generated when static_cast
  is put into fix, and no space were added before it.
- Fixed issue with duplicate parentheses being added when double
  implicit cast is used.
---
 .../ImplicitBoolConversionCheck.cpp           | 22 +++++++++++++++----
 clang-tools-extra/docs/ReleaseNotes.rst       |  6 +++--
 .../readability/implicit-bool-conversion.cpp  |  9 ++++++++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index 11706ffb5b7d4f4..097c9dcd62b5c78 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -151,16 +151,30 @@ StringRef getEquivalentBoolLiteralForExpr(const Expr *Expression,
   return {};
 }
 
+bool needsSpacePrefix(SourceLocation Loc, ASTContext &Context) {
+  SourceRange PrefixRange(Loc.getLocWithOffset(-1), Loc);
+  StringRef SpaceBeforeStmtStr = Lexer::getSourceText(
+      CharSourceRange::getCharRange(PrefixRange), Context.getSourceManager(),
+      Context.getLangOpts(), nullptr);
+  if (SpaceBeforeStmtStr.empty())
+    return true;
+
+  const StringRef AllowedCharacters(" \t\n\v\f\r(){}[]<>;,+=-|&~!^*/");
+  return SpaceBeforeStmtStr.rtrim(AllowedCharacters).size() ==
+         SpaceBeforeStmtStr.size();
+}
+
 void fixGenericExprCastFromBool(DiagnosticBuilder &Diag,
                                 const ImplicitCastExpr *Cast,
                                 ASTContext &Context, StringRef OtherType) {
   const Expr *SubExpr = Cast->getSubExpr();
-  bool NeedParens = !isa<ParenExpr>(SubExpr);
+  const bool NeedParens = !isa<ParenExpr>(SubExpr->IgnoreImplicit());
+  const bool NeedSpace = needsSpacePrefix(Cast->getBeginLoc(), Context);
 
   Diag << FixItHint::CreateInsertion(
-      Cast->getBeginLoc(),
-      (Twine("static_cast<") + OtherType + ">" + (NeedParens ? "(" : ""))
-          .str());
+      Cast->getBeginLoc(), (Twine() + (NeedSpace ? " " : "") + "static_cast<" +
+                            OtherType + ">" + (NeedParens ? "(" : ""))
+                               .str());
 
   if (NeedParens) {
     SourceLocation EndLoc = Lexer::getLocForEndOfToken(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d77267588db9158..38befdecd474eb6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -502,8 +502,10 @@ Changes in existing checks
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options. It also now provides more consistent
-  suggestions when parentheses are added to the return value or expressions. 
-  It also ignores false-positives for comparison containing bool bitfield.
+  suggestions when parentheses are added to the return value or expressions.
+  It also ignores false-positives for comparison containing ``bool`` bitfield.
+  Auto-fix suggestions creating invalid code in certain scenarios have now been
+  fixed.
 
 - Improved :doc:`readability-misleading-indentation
   <clang-tidy/checks/readability/misleading-indentation>` check to ignore
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
index 6f1f29729718875..07e4ca23dbb6bd1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
@@ -524,3 +524,12 @@ namespace PR71867 {
     // CHECK-FIXES: return (x ? 1 : 0) != 0;
   }
 }
+
+namespace PR71848 {
+  int fun() {
+    bool foo = false;
+    return( foo );
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
+// CHECK-FIXES: return static_cast<int>( foo );
+  }
+}

>From 0b06b06cd112cdd7aebdd1ea4398afbbf90d8876 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Thu, 18 Jan 2024 19:18:58 +0000
Subject: [PATCH 2/3] Reworked release notes

---
 clang-tools-extra/docs/ReleaseNotes.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 38befdecd474eb6..29cc5677c0c1ea2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -501,11 +501,11 @@ Changes in existing checks
 - Improved :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
   do-while loops into account for the `AllowIntegerConditions` and
-  `AllowPointerConditions` options. It also now provides more consistent
-  suggestions when parentheses are added to the return value or expressions.
-  It also ignores false-positives for comparison containing ``bool`` bitfield.
-  Auto-fix suggestions creating invalid code in certain scenarios have now been
-  fixed.
+  `AllowPointerConditions` options. Corrected issues with invalid fix
+  suggestions for ``static_cast`` without a preceding space. Addressed duplicate
+  parentheses in double implicit casts, ensuring more consistent suggestions for
+  added parentheses in return values or expressions. Also, ignored false
+  positives in comparisons with ``bool`` bitfields.
 
 - Improved :doc:`readability-misleading-indentation
   <clang-tidy/checks/readability/misleading-indentation>` check to ignore

>From f8a3b8931b4d34aa39b3f7bdbbe0fc9739043a89 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Thu, 18 Jan 2024 22:39:01 +0000
Subject: [PATCH 3/3] Fixes

---
 .../clang-tidy/readability/ImplicitBoolConversionCheck.cpp     | 3 +--
 .../checkers/readability/implicit-bool-conversion.cpp          | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index 097c9dcd62b5c78..4f02950e7794cbe 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -160,8 +160,7 @@ bool needsSpacePrefix(SourceLocation Loc, ASTContext &Context) {
     return true;
 
   const StringRef AllowedCharacters(" \t\n\v\f\r(){}[]<>;,+=-|&~!^*/");
-  return SpaceBeforeStmtStr.rtrim(AllowedCharacters).size() ==
-         SpaceBeforeStmtStr.size();
+  return !AllowedCharacters.contains(SpaceBeforeStmtStr.back());
 }
 
 void fixGenericExprCastFromBool(DiagnosticBuilder &Diag,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
index 07e4ca23dbb6bd1..d6e7dcc4d8867b7 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
@@ -529,7 +529,7 @@ namespace PR71848 {
   int fun() {
     bool foo = false;
     return( foo );
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
 // CHECK-FIXES: return static_cast<int>( foo );
   }
 }