ztamas updated this revision to Diff 232814.
ztamas added a comment.
Fixes small things mentioned by reviewer commments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71174/new/
https://reviews.llvm.org/D71174
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly caught by the check.
+
+int SimpleVarDeclaration() {
+ signed char CCharacter = -5;
+ int NCharacter = CCharacter;
+ // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast 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: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+int CStyleCast() {
+ signed char CCharacter = -5;
+ int NCharacter;
+ NCharacter = (int)CCharacter;
+ // CHECK-MESSAGES: [[@LINE-1]]:21: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+int StaticCast() {
+ signed char CCharacter = -5;
+ int NCharacter;
+ NCharacter = static_cast<int>(CCharacter);
+ // CHECK-MESSAGES: [[@LINE-1]]:33: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+int FunctionalCast() {
+ signed char CCharacter = -5;
+ int NCharacter;
+ NCharacter = int(CCharacter);
+ // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast 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: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+int CharPointer(signed char *CCharacter) {
+ int NCharacter = *CCharacter;
+ // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly ignored by the check.
+
+int UnsignedCharCast() {
+ unsigned char CCharacter = 'a';
+ int NCharacter = CCharacter;
+
+ return NCharacter;
+}
+
+int PositiveConstValue() {
+ const signed char CCharacter = 5;
+ int NCharacter = CCharacter;
+
+ return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of declaration expression.
+int DescendantCast() {
+ signed char CCharacter = 'a';
+ int NCharacter = 10 + CCharacter;
+
+ return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of assignment expression.
+int DescendantCastAssignment() {
+ signed char CCharacter = 'a';
+ int NCharacter;
+ NCharacter = 10 + CCharacter;
+
+ return NCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolVarDeclaration() {
+ signed char CCharacter = 'a';
+ bool BCharacter = CCharacter == 'b';
+
+ return BCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolAssignment() {
+ signed char CCharacter = 'a';
+ bool BCharacter;
+ BCharacter = CCharacter == 'b';
+
+ return BCharacter;
+}
+
+// char is an integer type in clang; make sure to ignore it.
+unsigned char CharToCharCast() {
+ signed char SCCharacter = 'a';
+ unsigned char USCharacter;
+ USCharacter = SCCharacter;
+
+ return USCharacter;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: bugprone-signed-char-misuse.CharTypdefsToIgnore, value: "sal_Int8;int8_t"}]}' \
+// RUN: --
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly caught by the check.
+
+// Check that a simple test case is still caught.
+int SimpleAssignment() {
+ signed char CCharacter = -5;
+ int NCharacter;
+ NCharacter = CCharacter;
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+typedef signed char sal_Char;
+
+int TypedefNotInIgnorableList() {
+ sal_Char CCharacter = 'a';
+ int NCharacter = CCharacter;
+ // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
+
+///////////////////////////////////////////////////////////////////
+/// Test cases correctly ignored by the check.
+
+typedef signed char sal_Int8;
+
+int OneIgnorableTypedef() {
+ sal_Int8 CCharacter = 'a';
+ int NCharacter = CCharacter;
+
+ return NCharacter;
+}
+
+typedef signed char int8_t;
+
+int OtherIgnorableTypedef() {
+ int8_t CCharacter = 'a';
+ int NCharacter = CCharacter;
+
+ return NCharacter;
+}
+
+///////////////////////////////////////////////////////////////////
+/// Test cases which should be caught by the check.
+
+namespace boost {
+
+template <class T>
+class optional {
+ T *member;
+
+public:
+ optional(T value) {
+ member = new T(value);
+ }
+
+ T operator*() { return *member; }
+};
+
+} // namespace boost
+
+int DereferenceWithTypdef(boost::optional<sal_Int8> param) {
+ int NCharacter = *param;
+ // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+ return NCharacter;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -63,6 +63,7 @@
bugprone-not-null-terminated-result
bugprone-parent-virtual-call
bugprone-posix-return
+ bugprone-signed-char-misuse
bugprone-sizeof-container
bugprone-sizeof-expression
bugprone-string-constructor
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - bugprone-signed-char-misuse
+
+bugprone-signed-char-misuse
+===========================
+
+Finds ``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. The human programmer probably
+expects that after an integer conversion the converted value matches with the
+character code (a value from [0..255]), however, the actual value is in
+[-128..127] interval. This also applies to the plain ``char`` type on
+those implementations which represent ``char`` similar to ``signed char``.
+
+To avoid this kind of misinterpretation, the desired way of converting from a
+``signed char`` to an integer value is converting to ``unsigned char`` first,
+which stores all the characters in the positive [0..255] interval which matches
+with the known character codes.
+
+By now, this check is limited to assignments and variable declarations,
+where a ``signed char`` is assigned to an integer variable. There are other
+use cases where the same misinterpretation might lead to similar bugous
+behavior.
+
+See also:
+`STR34-C. Cast characters to unsigned char before converting to larger integer sizes
+<https://wiki.sei.cmu.edu/confluence/display/c/STR34-C.+Cast+characters+to+unsigned+char+before+converting+to+larger+integer+sizes>`_
+
+A good example from the CERT description when a ``char`` variable is used to
+read from a file that might contain non-ASCII characters. The problem comes
+up when the code uses the ``-1`` integer value as EOF, while the 255 character
+code is also stored as ``-1`` in two's complement form of char type.
+See a simple example of this bellow. This code stops not only when it reaches
+the end of the file, but also when it gets a character with the 255 code.
+
+.. code-block:: c++
+
+ #define EOF (-1)
+
+ int read(void) {
+ char CChar;
+ int IChar = EOF;
+
+ if (readChar(CChar)) {
+ IChar = CChar;
+ }
+ return IChar;
+ }
+
+A proper way to fix the code above is converting the ``char`` variable to
+an ``unsigned char`` value first.
+
+.. code-block:: c++
+
+ #define EOF (-1)
+
+ int read(void) {
+ char CChar;
+ int IChar = EOF;
+
+ if (readChar(CChar)) {
+ IChar = static_cast<unsigned char>(CChar);
+ }
+ return IChar;
+ }
+
+.. 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.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
Without the null terminator it can result in undefined behaviour when the
string is read.
+- New :doc:`bugprone-signed-char-misuse
+ <clang-tidy/checks/bugprone-signed-char-misuse>` check.
+
+ Finds ``signed char`` -> integer conversions which might indicate a programming
+ error.
+
- New :doc:`cert-mem57-cpp
<clang-tidy/checks/cert-mem57-cpp>` check.
Index: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
@@ -0,0 +1,44 @@
+//===--- SignedCharMisuseCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds ``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. The human programmer probably
+/// expects that after an integer conversion the converted value matches with the
+/// character code (a value from [0..255]), however, the actual value is in
+/// [-128..127] interval. This also applies to the plain ``char`` type on
+/// those implementations which represent ``char`` similar to ``signed char``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signed-char-misuse.html
+class SignedCharMisuseCheck : public ClangTidyCheck {
+public:
+ SignedCharMisuseCheck(StringRef Name, ClangTidyContext *Context);
+
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ const std::string CharTypdefsToIgnoreList;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNEDCHARMISUSECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
@@ -0,0 +1,108 @@
+//===--- SignedCharMisuseCheck.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 "SignedCharMisuseCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+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()));
+}
+} // namespace
+
+SignedCharMisuseCheck::SignedCharMisuseCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ CharTypdefsToIgnoreList(Options.get("CharTypdefsToIgnore", "")) {}
+
+void SignedCharMisuseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "CharTypdefsToIgnore", CharTypdefsToIgnoreList);
+}
+
+void SignedCharMisuseCheck::registerMatchers(MatchFinder *Finder) {
+ // 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
+ // worry about the misinterpretation of char values.
+ const auto IntTypedef = qualType(
+ hasDeclaration(typedefDecl(hasAnyListedName(CharTypdefsToIgnoreList))));
+
+ const auto SignedCharType = expr(hasType(qualType(
+ allOf(isAnyCharacter(), isSignedInteger(), unless(IntTypedef)))));
+
+ const auto IntegerType = qualType(allOf(isInteger(), unless(isAnyCharacter()),
+ unless(booleanType())))
+ .bind("integerType");
+ ;
+
+ // We are interested in signed char -> integer conversion.
+ const auto ImplicitCastExpr =
+ implicitCastExpr(hasSourceExpression(SignedCharType),
+ hasImplicitDestinationType(IntegerType))
+ .bind("castExpression");
+ ;
+
+ const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
+ const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
+ const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+
+ // We catch any type of casts to an integer. We need to have these cast
+ // expressions explicitly to catch only those casts which are direct children
+ // of an assignment/declaration.
+ const auto CastExpr = expr(anyOf(ImplicitCastExpr, CStyleCastExpr,
+ StaticCastExpr, FunctionalCastExpr));
+
+ // Catch assignments with the suspicious type conversion.
+ const auto AssignmentOperatorExpr = expr(binaryOperator(
+ hasOperatorName("="), hasLHS(hasType(IntegerType)), hasRHS(CastExpr)));
+
+ Finder->addMatcher(AssignmentOperatorExpr, this);
+
+ // Catch declarations with the suspicious type conversion.
+ const auto Declaration =
+ varDecl(isDefinition(), hasType(IntegerType), hasInitializer(CastExpr));
+
+ Finder->addMatcher(Declaration, this);
+}
+
+void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *CastExpression =
+ Result.Nodes.getNodeAs<ImplicitCastExpr>("castExpression");
+ const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType");
+ assert(CastExpression);
+ assert(IntegerType);
+
+ // Ignore the match if we know that the value is not negative.
+ // The potential misinterpretation happens for negative values only.
+ Expr::EvalResult EVResult;
+ if (!CastExpression->isValueDependent() &&
+ CastExpression->getSubExpr()->EvaluateAsInt(EVResult, *Result.Context)) {
+ llvm::APSInt Value1 = EVResult.Val.getInt();
+ if (Value1.isNonNegative())
+ return;
+ }
+
+ diag(CastExpression->getBeginLoc(),
+ "singed char -> integer (%0) conversion; "
+ "consider to cast to unsigned char first.")
+ << *IntegerType;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -28,6 +28,7 @@
NotNullTerminatedResultCheck.cpp
ParentVirtualCallCheck.cpp
PosixReturnCheck.cpp
+ SignedCharMisuseCheck.cpp
SizeofContainerCheck.cpp
SizeofExpressionCheck.cpp
StringConstructorCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -36,6 +36,7 @@
#include "NotNullTerminatedResultCheck.h"
#include "ParentVirtualCallCheck.h"
#include "PosixReturnCheck.h"
+#include "SignedCharMisuseCheck.h"
#include "SizeofContainerCheck.h"
#include "SizeofExpressionCheck.h"
#include "StringConstructorCheck.h"
@@ -119,6 +120,8 @@
"bugprone-parent-virtual-call");
CheckFactories.registerCheck<PosixReturnCheck>(
"bugprone-posix-return");
+ CheckFactories.registerCheck<SignedCharMisuseCheck>(
+ "bugprone-signed-char-misuse");
CheckFactories.registerCheck<SizeofContainerCheck>(
"bugprone-sizeof-container");
CheckFactories.registerCheck<SizeofExpressionCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
