[clang-tools-extra] 030ff90 - [clang-tidy] extend bugprone-signed-char-misuse check with array subscript case.
Tamás Zolnai via cfe-commits
cfe-commits at lists.llvm.org
Sat May 2 05:08:56 PDT 2020
Author: Tamás Zolnai
Date: 2020-05-02T14:05:05+02:00
New Revision: 030ff901f43258d18b68a77b0085d0fae2a0fbc6
URL: https://github.com/llvm/llvm-project/commit/030ff901f43258d18b68a77b0085d0fae2a0fbc6
DIFF: https://github.com/llvm/llvm-project/commit/030ff901f43258d18b68a77b0085d0fae2a0fbc6.diff
LOG: [clang-tidy] extend bugprone-signed-char-misuse check with array subscript case.
Summary:
To cover STR34-C rule's second use case, where ``signed char`` is
used for array subscript after an integer conversion. In the case
of non-ASCII character this conversion will result in a value
in excess of UCHAR_MAX.
There is another clang-tidy check which catches these cases.
cppcoreguidelines-pro-bounds-constant-array-index catches any
indexing which is not integer constant. I think this check is
very strict about the index (e.g. constant), so it's still useful
to cover the ``signed char`` use case in this check, so we
can provide a way to catch the SEI cert rule's use cases on a
codebase, where this CPP guideline is not used.
Reviewers: aaron.ballman, njames93
Reviewed By: aaron.ballman
Subscribers: xazax.hun, cfe-commits
Tags: #clang, #clang-tools-extra
Differential Revision: https://reviews.llvm.org/D78904
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
index 732ccbc9dd2a..66f00e35c7e7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
@@ -102,11 +102,31 @@ void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
.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);
+
+ // Matcher for std arrays.
+ const auto STDArraySubscript =
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("[]"),
+ hasArgument(0, hasType(cxxRecordDecl(hasName("::std::array")))),
+ hasArgument(1, SignedCharCastExpr))
+ .bind("arraySubscript");
+
+ Finder->addMatcher(STDArraySubscript, this);
}
void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) {
const auto *SignedCastExpression =
Result.Nodes.getNodeAs<ImplicitCastExpr>("signedCastExpression");
+ const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType");
+ assert(SignedCastExpression);
+ assert(IntegerType);
// Ignore the match if we know that the signed char's value is not negative.
// The potential misinterpretation happens for negative values only.
@@ -135,14 +155,17 @@ void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) {
diag(Comparison->getBeginLoc(),
"comparison between 'signed char' and 'unsigned char'");
- } else if (const auto *IntegerType =
- Result.Nodes.getNodeAs<QualType>("integerType")) {
+ } else if (Result.Nodes.getNodeAs<Expr>("arraySubscript")) {
+ diag(SignedCastExpression->getBeginLoc(),
+ "'signed char' to %0 conversion in array subscript; "
+ "consider casting to 'unsigned char' first.")
+ << *IntegerType;
+ } else {
diag(SignedCastExpression->getBeginLoc(),
"'signed char' to %0 conversion; "
"consider casting to 'unsigned char' first.")
<< *IntegerType;
- } else
- llvm_unreachable("Unexpected match");
+ }
}
} // namespace bugprone
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 e3ecf75a3a52..c2bd2df062b1 100644
--- 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
@@ -31,11 +31,10 @@ It depends on the actual platform whether plain ``char`` is handled as ``signed
by default and so it is caught by this check or not. To change the default behavior
you can use ``-funsigned-char`` and ``-fsigned-char`` compilation options.
-Currently, this check is limited to assignments and variable declarations,
-where a ``signed char`` is assigned to an integer variable and to
-equality/inequality comparisons between ``signed char`` and ``unsigned char``.
-There are other use cases where the unexpected value ranges might lead to
-similar bogus behavior.
+Currently, this check warns in the following cases:
+- ``signed char`` is assigned to an integer variable
+- ``signed char`` and ``unsigned char`` are compared with equality/inequality operator
+- ``signed char`` is converted to an integer in the array subscript
See also:
`STR34-C. Cast characters to unsigned char before converting to larger integer sizes
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
index ef42b0c85ae6..a1cc2ae48991 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
@@ -3,6 +3,16 @@
///////////////////////////////////////////////////////////////////
/// Test cases correctly caught by the check.
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename T, size_t N>
+struct array {
+ T &operator[](size_t n);
+ T &at(size_t n);
+};
+} // namespace std
+
int SimpleVarDeclaration() {
signed char CCharacter = -5;
int NCharacter = CCharacter;
@@ -90,6 +100,16 @@ int CompareWithUnsignedNonAsciiConstant(signed char SCharacter) {
return 0;
}
+int SignedCharCArraySubscript(signed char SCharacter) {
+ int Array[3] = {1, 2, 3};
+
+ return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+}
+
+int SignedCharSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) {
+ return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+}
+
///////////////////////////////////////////////////////////////////
/// Test cases correctly ignored by the check.
@@ -207,3 +227,23 @@ int CompareWithUnsignedAsciiConstant(signed char SCharacter) {
return 1;
return 0;
}
+
+int UnsignedCharCArraySubscript(unsigned char USCharacter) {
+ int Array[3] = {1, 2, 3};
+
+ return Array[static_cast<unsigned int>(USCharacter)];
+}
+
+int CastedCArraySubscript(signed char SCharacter) {
+ int Array[3] = {1, 2, 3};
+
+ return Array[static_cast<unsigned char>(SCharacter)];
+}
+
+int UnsignedCharSTDArraySubscript(std::array<int, 3> Array, unsigned char USCharacter) {
+ return Array[static_cast<unsigned int>(USCharacter)];
+}
+
+int CastedSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) {
+ return Array[static_cast<unsigned char>(SCharacter)];
+}
More information about the cfe-commits
mailing list