Index: clang-tidy/cppcoreguidelines/CMakeLists.txt =================================================================== --- clang-tidy/cppcoreguidelines/CMakeLists.txt +++ clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -5,6 +5,7 @@ CppCoreGuidelinesTidyModule.cpp InterfacesGlobalInitCheck.cpp MacroUsageCheck.cpp + MixedIntArithmeticCheck.cpp NarrowingConversionsCheck.cpp NoMallocCheck.cpp OwningMemoryCheck.cpp Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp =================================================================== --- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -17,6 +17,7 @@ #include "AvoidGotoCheck.h" #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" +#include "MixedIntArithmeticCheck.h" #include "NarrowingConversionsCheck.h" #include "NoMallocCheck.h" #include "OwningMemoryCheck.h" @@ -51,6 +52,8 @@ "cppcoreguidelines-interfaces-global-init"); CheckFactories.registerCheck( "cppcoreguidelines-macro-usage"); + CheckFactories.registerCheck( + "cppcoreguidelines-mixed-int-arithmetic"); CheckFactories.registerCheck( "cppcoreguidelines-narrowing-conversions"); CheckFactories.registerCheck("cppcoreguidelines-no-malloc"); Index: clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.h =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.h @@ -0,0 +1,35 @@ +//===--- MixedIntArithmeticCheck.h - clang-tidy------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MIXED_INT_ARITHMETIC_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MIXED_INT_ARITHMETIC_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/// Find places that use unsigned integers in arithmetic expressions. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-mixed-int-arithmetic.html +class MixedIntArithmeticCheck : public ClangTidyCheck { +public: + MixedIntArithmeticCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MIXED_INT_ARITHMETIC_H Index: clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp @@ -0,0 +1,65 @@ +//===--- MixedIntArithmeticCheck.cpp - clang-tidy--------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MixedIntArithmeticCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +void MixedIntArithmeticCheck::registerMatchers(MatchFinder *Finder) { + const auto UnsignedIntegerOperand = + expr(ignoringImpCasts(hasType(isUnsignedInteger()))) + .bind("unsigned-binary-operand"); + const auto SignedIntegerOperand = + expr(ignoringImpCasts(hasType(isSignedInteger()))) + .bind("signed-binary-operand"); + + // Match integer arithmetic mixing signed and unsigned types. + Finder->addMatcher( + binaryOperator(allOf(anyOf(hasOperatorName("+"), hasOperatorName("-"), + hasOperatorName("*"), hasOperatorName("/")), + hasEitherOperand(UnsignedIntegerOperand), + hasEitherOperand(SignedIntegerOperand), + hasRHS(hasType(isInteger())), + hasLHS(hasType(isInteger())))) + .bind("mixed-binary-arithmetic"), + this); +} + +void MixedIntArithmeticCheck::check(const MatchFinder::MatchResult &Result) { + const auto *UnsignedOperand = + Result.Nodes.getNodeAs("unsigned-binary-operand"); + const auto *SignedOperand = + Result.Nodes.getNodeAs("signed-binary-operand"); + const auto *MixedArithmetic = + Result.Nodes.getNodeAs("mixed-binary-arithmetic"); + + assert(UnsignedOperand && MixedArithmetic && + "Matcher did not match operand and operation"); + + diag(MixedArithmetic->getLocStart(), + "mixed signed and unsigned arithmetic; " + "prefer signed integers and use unsigned types only for modulo " + "arithmetic") + << MixedArithmetic->getSourceRange(); + + diag(SignedOperand->getLocStart(), "signed operand", DiagnosticIDs::Note) + << SignedOperand->getSourceRange(); + diag(UnsignedOperand->getLocStart(), "unsigned operand", DiagnosticIDs::Note) + << UnsignedOperand->getSourceRange(); +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -188,6 +188,13 @@ Finds macro usage that is considered problematic because better language constructs exist for the task. +- New :doc:`cppcoreguidelines-mixed-int-arithmetic + ` check. + + Finds cases where unsigned and signed integers are used together in arithmetic expressions. + Unsigned integers wrap to 0 when overflowing while the behaviour of signed integers + is undefined in this case. The combination leads to possibly unexpected results. + - New :doc:`google-objc-function-naming ` check. @@ -237,6 +244,7 @@ Checks for functions with a ``const``-qualified return type and recommends removal of the ``const`` keyword. +>>>>>>> master - New :doc:`readability-magic-numbers ` check. Index: docs/clang-tidy/checks/cppcoreguidelines-mixed-int-arithmetic.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/cppcoreguidelines-mixed-int-arithmetic.rst @@ -0,0 +1,38 @@ +.. title:: clang-tidy - cppcoreguidelines-mixed-int-arithmetic + +cppcoreguidelines-mixed-int-arithmetic +====================================== + +This check enforces `ES. 100 `_ +and `ES. 102 `_ +of the `CppCoreGuidelines `_ +addressing the use of unsigned types in integer arithmetic. + +Because of the subtile difference between ``signed`` and ``unsigned`` integer +types in C++ it is recommended to use ``signed`` types in general for arithmetic +and to not mix ``signed`` and ``unsigned`` integers in arithmetic expressions. + +The behaviour of ``signed`` integer type is undefined when an overflow occurs. +On the contrary ``unsigned`` types will wrap around leading to potentially +unexpected results of integer computations. + +.. code-block:: c++ + + // Taken from the CppCoreGuidelines + template + T subtract(T x, T2 y) + { + return x - y; + } + + void test() + { + int s = 5; + unsigned int us = 5; + cout << subtract(s, 7) << '\n'; // -2 + cout << subtract(us, 7u) << '\n'; // 4294967294 + cout << subtract(s, 7u) << '\n'; // -2 + cout << subtract(us, 7) << '\n'; // 4294967294 + cout << subtract(s, us + 2) << '\n'; // -2 + cout << subtract(us, s + 2) << '\n'; // 4294967294 + } Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -97,6 +97,7 @@ cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) cppcoreguidelines-interfaces-global-init cppcoreguidelines-macro-usage + cppcoreguidelines-mixed-int-arithmetic cppcoreguidelines-narrowing-conversions cppcoreguidelines-no-malloc cppcoreguidelines-non-private-member-variables-in-classes (redirects to misc-non-private-member-variables-in-classes) Index: test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp =================================================================== --- /dev/null +++ test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp @@ -0,0 +1,188 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-mixed-int-arithmetic %t + +enum UnsignedEnum : unsigned char { + UEnum1, + UEnum2 +}; + +enum SignedEnum : signed char { + SEnum1, + SEnum2 +}; + +unsigned char returnUnsignedCharacter() { return 42; } +unsigned returnUnsignedNumber() { return 42u; } +long returnBigNumber() { return 42; } +float unrelatedThing() { return 42.f; } +SignedEnum returnSignedEnum() { return SEnum1; } +UnsignedEnum returnUnsignedEnum() { return UEnum1; } + +void mixed_binary() { + unsigned int UInt1 = 42; + signed int SInt1 = 42; + UnsignedEnum UE1 = UEnum1; + SignedEnum SE1 = SEnum1; + float UnrelatedFloat = 42.f; + + // Test traditional integer types. + auto R1 = UInt1 + SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand + + int R2 = UInt1 - SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:20: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:12: note: unsigned operand + + unsigned int R3 = UInt1 * SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand + + unsigned int R4 = UInt1 / returnBigNumber(); + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand + + char R5 = returnUnsignedCharacter() + SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:41: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand + + auto R6 = SInt1 - 10u; + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand + + auto R7 = UInt1 * 10; + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand + + auto R8 = 10u / returnBigNumber(); + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:19: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand + + auto R9 = 10 + returnUnsignedCharacter(); + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:18: note: unsigned operand + + // Test enum types. + char R10 = returnUnsignedEnum() - SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:37: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand + + unsigned char R11 = returnSignedEnum() * UInt1; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:23: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:44: note: unsigned operand + + char R12 = UE1 / SInt1; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:20: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand + + unsigned char R13 = SE1 + UInt1; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:23: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:29: note: unsigned operand + + auto R14 = SE1 - 10u; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:14: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:20: note: unsigned operand + + auto R15 = UE1 * 10; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:20: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand + + auto R16 = returnSignedEnum() / 10u; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:14: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:35: note: unsigned operand + + auto R17 = returnUnsignedEnum() + 10; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic + // CHECK-MESSAGES: [[@LINE-2]]:37: note: signed operand + // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand + + // Check that floating pointer numbers do not interfere improperly. + // Implicit conversion from float to int are covered in other checks. + int Ok1 = UInt1 + UnrelatedFloat; + unsigned int Ok2 = SInt1 - UnrelatedFloat; + int Ok3 = UInt1 * unrelatedThing(); + unsigned int Ok4 = SInt1 / unrelatedThing(); + auto Ok5 = 10 + UnrelatedFloat; + auto Ok6 = 10u - UnrelatedFloat; +} + +void mixed_assignments() { +} + +void pure_unsigned() { + unsigned int UInt1 = 42u; + unsigned char UChar1 = 42u; + UnsignedEnum UE1 = UEnum1; + float UnrelatedFloat = 42.f; + + auto Ok1 = UInt1 + UChar1; + auto Ok2 = UChar1 + UInt1; + auto Ok3 = UInt1 + returnUnsignedCharacter(); + auto Ok4 = UChar1 + returnUnsignedCharacter(); + auto Ok5 = returnUnsignedCharacter() + returnUnsignedCharacter(); + auto Ok6 = UInt1 + UE1; + auto Ok7 = UInt1 + returnUnsignedEnum(); + auto Ok8 = UE1 + UE1; + // FIXME: unsigned character converts to 'int' and pollutes the result. + // http://en.cppreference.com/w/cpp/language/implicit_conversion Integral conversions + // if `returnUnsignedCharacter()` would return char, the conversion would result + // in either `signed int` or `unsigned int` (arch dependent i guess). Both `short` + // and `char` perform this conversion in arithmetic operations. This would probably + // need some bigger magic to match in the AST, but should be possible in theory. + auto Ok9 = 10u * (returnUnsignedNumber() + returnUnsignedEnum()); + auto Ok10 = 10u * (10u + 10ul); + auto Ok11 = 10u * (10u + returnUnsignedEnum()); + auto Ok12 = returnUnsignedCharacter() * (10u + returnUnsignedEnum()); + + // Test that unrelated types do not interfere. + auto OkUnrelated1 = UInt1 + UnrelatedFloat; + auto OkUnrelated2 = UChar1 + unrelatedThing(); + auto OkUnrelated3 = UE1 + UnrelatedFloat; + auto OkUnrelated4 = UE1 + unrelatedThing(); + + // Test that correct assignments to not cause warnings. + UInt1 += 1u; + UInt1 -= returnUnsignedCharacter(); + UInt1 *= UE1; + UInt1 /= returnUnsignedEnum(); + UInt1 += (returnUnsignedCharacter() + returnUnsignedEnum()); +} + +void pure_signed() { + int SInt1 = 42u; + signed char SChar1 = 42u; + SignedEnum SE1 = SEnum1; + + float UnrelatedFloat = 42.f; + + auto Ok1 = SInt1 + SChar1; + auto Ok2 = SChar1 + SInt1; + auto Ok3 = SInt1 + returnBigNumber(); + auto Ok4 = SChar1 + returnBigNumber(); + auto Ok5 = returnBigNumber() + returnBigNumber(); + auto Ok6 = SInt1 + SE1; + auto Ok7 = SInt1 + returnSignedEnum(); + auto Ok8 = SE1 + SE1; + auto Ok9 = 10 * (returnBigNumber() + returnSignedEnum()); + + // Test that unrelated types do not interfere. + auto OkUnrelated1 = SInt1 + UnrelatedFloat; + auto OkUnrelated2 = SChar1 + unrelatedThing(); + auto OkUnrelated3 = SE1 + UnrelatedFloat; + auto OkUnrelated4 = SE1 + unrelatedThing(); +}