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

[ztamas]

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.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

33

Same request for the user-facing option name as above.

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h

41

I'd prefer a more descriptive name than this -- most people reading the code won't be familiar with that coding standard. How about DiagnoseSignedUnsignedCharComparisons or something along those lines?

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

117–119

The documentation should describe the actual differences rather than defer directly to other documentation. It's fine to make the description light on content and say "see <link> for more information" if the differences are hard to spell out concisely.

[ztamas]

CertSTR34C -> DiagnoseSignedUnsignedCharComparisons

[ztamas]

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h

41

OK, I updated the code to use this option name.

[aaron.ballman]

LGTM aside from some small nits with the documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

117

non-zero -> nonzero
comparison -> comparisons,
(with the plural and comma)

118

this use case is -> these comparisons are

clang-tools-extra/docs/clang-tidy/checks/cert-str34-c.rst

7

The underlining here looks too long.

[ztamas]

Documentation fixes

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h

 
 private:
   ast_matchers::internal::BindableMatcher<clang::Stmt> charCastExpression(
       bool IsSigned,
       const ast_matchers::internal::Matcher<clang::QualType> &IntegerType,
       const std::string &CastBindName) const;
 
   const std::string CharTypdefsToIgnoreList;
+  const bool DiagnoseSignedUnsignedCharComparisons;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H

clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp

static Matcher<TypedefDecl> hasAnyListedName(const std::string &Names) {
   const std::vector<std::string> NameList =
       utils::options::parseStringList(Names);
   return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
 }
 
 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.
 BindableMatcher<clang::Stmt> SignedCharMisuseCheck::charCastExpression(
     bool IsSigned, const Matcher<clang::QualType> &IntegerType,
     const std::string &CastBindName) const {
   // We can ignore typedefs which are some kind of integer types
   // (e.g. typedef char sal_Int8). In this case, we don't need to
void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(AssignmentOperatorExpr, this);
 
   // Catch declarations with singed char -> integer conversion.
   const auto Declaration = varDecl(isDefinition(), hasType(IntegerType),
                                    hasInitializer(SignedCharCastExpr));
 
   Finder->addMatcher(Declaration, 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.
   const auto CArraySubscript =
       arraySubscriptExpr(hasIndex(SignedCharCastExpr)).bind("arraySubscript");
 
   Finder->addMatcher(CArraySubscript, this);
 

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 //===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #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"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "../performance/MoveConstructorInitCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
     // POS
     CheckFactories.registerCheck<bugprone::BadSignalToKillThreadCheck>(
         "cert-pos44-c");
+    // STR
+    CheckFactories.registerCheck<bugprone::SignedCharMisuseCheck>(
+        "cert-str34-c");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     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;
   }
 };
 
 } // namespace cert
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<cert::CERTModule>

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-dcl37-c
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 - New alias :doc:`cert-dcl51-cpp
   <clang-tidy/checks/cert-dcl51-cpp>` to
   :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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`readability-qualified-auto
   <clang-tidy/checks/readability-qualified-auto>` check now supports a
   `AddConstToQualified` to enable adding ``const`` qualifiers to variables
   typed with ``auto *`` and ``auto &``.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

 .. title:: clang-tidy - bugprone-signed-char-misuse
 
 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
 misunderstanding of the written code both when an explicit and when an
 implicit conversion happens.
 
 When the code contains an explicit ``signed char`` -> integer conversion, the
 human programmer probably expects that the converted value matches with the

 
 .. option:: CharTypdefsToIgnore
 
   A semicolon-separated list of typedef names. In this list, we can list
   typedefs for ``char`` or ``signed char``, which will be ignored by the
   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``.

clang-tools-extra/docs/clang-tidy/checks/cert-str34-c.rst

+.. 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.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table:: Aliases..
    `cert-err09-cpp <cert-err09-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `cert-err61-cpp <cert-err61-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `cert-fio38-c <cert-fio38-c.html>`_, `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `cert-msc30-c <cert-msc30-c.html>`_, `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc32-c <cert-msc32-c.html>`_, `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `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>`_,
    `clang-analyzer-core.NullDereference <clang-analyzer-core.NullDereference.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.StackAddressEscape <clang-analyzer-core.StackAddressEscape.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.UndefinedBinaryOperatorResult <clang-analyzer-core.UndefinedBinaryOperatorResult.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.VLASize <clang-analyzer-core.VLASize.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,
    `clang-analyzer-core.uninitialized.ArraySubscript <clang-analyzer-core.uninitialized.ArraySubscript.html>`_, `Clang Static Analyzer <https://clang.llvm.org/docs/analyzer/checkers.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cert-str34-c.cpp

+// 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;
+}