[clang-tidy]: Add cert-str34-c alias for bugprone-signed-char-misuse.

Summary:
Added `DiagnoseSignedUnsignedCharComparisons` option to
filter out unrelated use cases. The SEI cert catches explicit
integer casts (two use cases), while in the case of
`signed char` \ `unsigned char` comparison, we have implicit
conversions.

Reviewers: aaron.ballman

Reviewed By: aaron.ballman

Subscribers: xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D79334

diff --git a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
index 66f00e3..3f72e5d 100644 (file)
--- a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
@@ -29,10 +29,14 @@ static Matcher<TypedefDecl> hasAnyListedName(const std::string &Names) {
 SignedCharMisuseCheck::SignedCharMisuseCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      CharTypdefsToIgnoreList(Options.get("CharTypdefsToIgnore", "")) {}
+      CharTypdefsToIgnoreList(Options.get("CharTypdefsToIgnore", "")),
+      DiagnoseSignedUnsignedCharComparisons(
+          Options.get("DiagnoseSignedUnsignedCharComparisons", true)) {}
 
 void SignedCharMisuseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CharTypdefsToIgnore", CharTypdefsToIgnoreList);
+  Options.store(Opts, "DiagnoseSignedUnsignedCharComparisons",
+                DiagnoseSignedUnsignedCharComparisons);
 }
 
 // Create a matcher for char -> integer cast.
@@ -92,16 +96,18 @@ void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(Declaration, this);
 
-  // Catch signed char/unsigned char comparison.
-  const auto CompareOperator =
-      expr(binaryOperator(hasAnyOperatorName("==", "!="),
-                          anyOf(allOf(hasLHS(SignedCharCastExpr),
-                                      hasRHS(UnSignedCharCastExpr)),
-                                allOf(hasLHS(UnSignedCharCastExpr),
-                                      hasRHS(SignedCharCastExpr)))))
-          .bind("comparison");
-
-  Finder->addMatcher(CompareOperator, this);
+  if (DiagnoseSignedUnsignedCharComparisons) {
+    // Catch signed char/unsigned char comparison.
+    const auto CompareOperator =
+        expr(binaryOperator(hasAnyOperatorName("==", "!="),
+                            anyOf(allOf(hasLHS(SignedCharCastExpr),
+                                        hasRHS(UnSignedCharCastExpr)),
+                                  allOf(hasLHS(UnSignedCharCastExpr),
+                                        hasRHS(SignedCharCastExpr)))))
+            .bind("comparison");
+
+    Finder->addMatcher(CompareOperator, this);
+  }
 
   // Catch array subscripts with signed char -> integer conversion.
   // Matcher for C arrays.

diff --git a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
index 3fa0c9c..84d3bbb 100644 (file)
--- a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
@@ -38,6 +38,7 @@ private:
       const std::string &CastBindName) const;
 
   const std::string CharTypdefsToIgnoreList;
+  const bool DiagnoseSignedUnsignedCharComparisons;
 };
 
 } // namespace bugprone

diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
index 6459dcf..6592d22 100644 (file)
--- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
+#include "../bugprone/SignedCharMisuseCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
@@ -108,6 +109,9 @@ public:
     // POS
     CheckFactories.registerCheck<bugprone::BadSignalToKillThreadCheck>(
         "cert-pos44-c");
+    // STR
+    CheckFactories.registerCheck<bugprone::SignedCharMisuseCheck>(
+        "cert-str34-c");
   }
 
   ClangTidyOptions getModuleOptions() override {
@@ -115,6 +119,7 @@ public:
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
     Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
     Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0";
+    Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "0";
     return Options;
   }
 };

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 97ef427..9e4fa62 100644 (file)
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -168,6 +168,11 @@ New check aliases
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
+- New alias :doc:`cert-str34-c
+  <clang-tidy/checks/cert-str34-c>` to
+  :doc:`bugprone-signed-char-misuse
+  <clang-tidy/checks/bugprone-signed-char-misuse>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 

diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
index c2bd2df..2894495 100644 (file)
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
@@ -3,6 +3,9 @@
 bugprone-signed-char-misuse
 ===========================
 
+`cert-str34-c` redirects here as an alias for this check. For the CERT alias,
+the `DiagnoseSignedUnsignedCharComparisons` option is set to `0`.
+
 Finds those ``signed char`` -> integer conversions which might indicate a
 programming error. The basic problem with the ``signed char``, that it might
 store the non-ASCII characters as negative values. This behavior can cause a
@@ -108,3 +111,8 @@ so both arguments will have the same type.
   check. This is useful when a typedef introduces an integer alias like
   ``sal_Int8`` or ``int8_t``. In this case, human misinterpretation is not
   an issue.
+
+.. option:: DiagnoseSignedUnsignedCharComparisons
+
+  When nonzero, the check will warn on ``signed char``/``unsigned char`` comparisons,
+  otherwise these comparisons are ignored. By default, this option is set to ``1``.

diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert-str34-c.rst b/clang-tools-extra/docs/clang-tidy/checks/cert-str34-c.rst
new file mode 100644 (file)
index 0000000..f28311b
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/cert-str34-c.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-str34-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-signed-char-misuse.html
+
+cert-str34-c
+============
+
+The cert-str34-c check is an alias, please see
+`bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_
+for more information.

diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index c3d047f..b4d09d5 100644 (file)
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -320,6 +320,7 @@ Clang-Tidy Checks
    `cert-oop11-cpp <cert-oop11-cpp.html>`_, `performance-move-constructor-init <performance-move-constructor-init.html>`_, "Yes"
    `cert-oop54-cpp <cert-oop54-cpp.html>`_, `bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_,
    `cert-pos44-c <cert-pos44-c.html>`_, `bugprone-bad-signal-to-kill-thread <bugprone-bad-signal-to-kill-thread.html>`_,
+   `cert-str34-c <cert-str34-c.html>`_, `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `clang-analyzer-core.CallAndMessage <clang-analyzer-core.CallAndMessage.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.DivideZero <clang-analyzer-core.DivideZero.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.NonNullParamChecker <clang-analyzer-core.NonNullParamChecker.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,

diff --git a/clang-tools-extra/test/clang-tidy/checkers/cert-str34-c.cpp b/clang-tools-extra/test/clang-tidy/checkers/cert-str34-c.cpp
new file mode 100644 (file)
index 0000000..2257066
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cert-str34-c.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cert-str34-c %t
+
+// Check whether alias is actually working.
+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. [cert-str34-c]
+
+  return NCharacter;
+}
+
+// Check whether bugprone-signed-char-misuse.DiagnoseSignedUnsignedCharComparisons option is set correctly.
+int SignedUnsignedCharEquality(signed char SCharacter) {
+  unsigned char USCharacter = 'a';
+  if (SCharacter == USCharacter) // no warning
+    return 1;
+  return 0;
+}