[clang-tools-extra] [clang-tidy] Fix false positives in `bugprone-signed-char-misuse` (PR #149790)

[via cfe-commits]

=?utf-8?q?Björn?= Svensson <bjorn.a.svensson at est.tech>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/149790 at github.com>


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Björn Svensson (bjosv)

<details>
<summary>Changes</summary>

Ignore false positives on C23 enums which allows setting the fixed underlying type to signed char.

The AST tree for [C enums](https://godbolt.org/z/5snW9114E) includes a ImplicitCastExp (that was matched) but this is not the case for [C++ enums](https://godbolt.org/z/Yj5TaqzcE).

Fixes #<!-- -->145651

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


3 Files Affected:

- (modified) clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp (+21-5) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/signed-char-misuse-c23.c (+91) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
index ea3b8b8e9df4f..82276628203dd 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
@@ -16,6 +16,17 @@ using namespace clang::ast_matchers::internal;
 
 namespace clang::tidy::bugprone {
 
+namespace {
+
+AST_MATCHER(Stmt, isC23Stmt) {
+  return Finder->getASTContext().getLangOpts().C23;
+}
+AST_MATCHER(Decl, isC23Decl) {
+  return Finder->getASTContext().getLangOpts().C23;
+}
+
+} // anonymous namespace
+
 static constexpr int UnsignedASCIIUpperBound = 127;
 
 SignedCharMisuseCheck::SignedCharMisuseCheck(StringRef Name,
@@ -75,16 +86,21 @@ void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
   const auto UnSignedCharCastExpr =
       charCastExpression(false, IntegerType, "unsignedCastExpression");
 
-  // Catch assignments with signed char -> integer conversion.
+  // Catch assignments with signed char -> integer conversion. Ignore false
+  // positives on C23 enums with the fixed underlying type of signed char.
   const auto AssignmentOperatorExpr =
       expr(binaryOperator(hasOperatorName("="), hasLHS(hasType(IntegerType)),
-                          hasRHS(SignedCharCastExpr)));
+                          hasRHS(SignedCharCastExpr)),
+           unless(allOf(isC23Stmt(), binaryOperator(hasLHS(hasType(
+                                         hasCanonicalType(enumType())))))));
 
   Finder->addMatcher(AssignmentOperatorExpr, this);
 
-  // Catch declarations with signed char -> integer conversion.
-  const auto Declaration = varDecl(isDefinition(), hasType(IntegerType),
-                                   hasInitializer(SignedCharCastExpr));
+  // Catch declarations with signed char -> integer conversion. Ignore false
+  // positives on C23 enums with the fixed underlying type of signed char.
+  const auto Declaration = varDecl(
+      isDefinition(), hasType(IntegerType), hasInitializer(SignedCharCastExpr),
+      unless(allOf(isC23Decl(), hasType(hasCanonicalType(enumType())))));
 
   Finder->addMatcher(Declaration, this);
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9eb3835fe8340..af7192c4d649a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -204,6 +204,10 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/optional-value-conversion>` check to detect
   conversion in argument of ``std::make_optional``.
 
+- Improved :doc:`bugprone-signed-char-misuse
+  <clang-tidy/checks/bugprone/signed-char-misuse>` check by fixing
+  false positives on C23 enums with the fixed underlying type of signed char.
+
 - Improved :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone/sizeof-expression>` check by adding
   `WarnOnSizeOfInLoopTermination` option to detect misuses of ``sizeof``
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/signed-char-misuse-c23.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/signed-char-misuse-c23.c
new file mode 100644
index 0000000000000..349843f42fbc7
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/signed-char-misuse-c23.c
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t -- -- -std=c23
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly caught by the check.
+
+int SimpleVarDeclaration() {
+  signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int SimpleAssignment() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int NegativeConstValue() {
+  const signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int CharPointer(signed char *CCharacter) {
+  int NCharacter = *CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int SignedUnsignedCharEquality(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter == USCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int SignedUnsignedCharIneqiality(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter != USCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int CompareWithNonAsciiConstant(unsigned char USCharacter) {
+  const signed char SCharacter = -5;
+  if (USCharacter == SCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+int CompareWithUnsignedNonAsciiConstant(signed char SCharacter) {
+  const unsigned char USCharacter = 128;
+  if (USCharacter == SCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
+    return 1;
+  return 0;
+}
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly ignored by the check.
+
+enum EType1 : signed char {
+  EType1_M128 = -128,
+  EType1_1 = 1,
+};
+
+enum EType1 es1_1 = EType1_M128;
+enum EType1 es1_2 = EType1_1;
+enum EType1 es1_3 = -128;
+
+void assign(enum EType1 *es) {
+    *es = EType1_M128;
+}
+
+
+typedef signed char int8_t;
+typedef enum EType2 : int8_t {
+  EType2_M128 = -128,
+  EType2_1 = 1,
+} EType2_t;
+
+EType2_t es2_1 = EType2_M128;
+EType2_t es2_2 = EType2_1;
+EType2_t es2_3 = -128;

``````````

</details>


https://github.com/llvm/llvm-project/pull/149790