Index: clang-tidy/CMakeLists.txt =================================================================== --- clang-tidy/CMakeLists.txt +++ clang-tidy/CMakeLists.txt @@ -27,6 +27,7 @@ add_subdirectory(tool) add_subdirectory(plugin) +add_subdirectory(bugprone) add_subdirectory(boost) add_subdirectory(cert) add_subdirectory(llvm) Index: clang-tidy/bugprone/BoolToIntegerConversionCheck.h =================================================================== --- /dev/null +++ clang-tidy/bugprone/BoolToIntegerConversionCheck.h @@ -0,0 +1,39 @@ +//===--- BoolToIntegerConversionCheck.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_MODERNIZE_BOOL_TO_INTEGER_CONVERSION_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_BOOL_TO_INTEGER_CONVERSION_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds implicit casts of bool literal to integer types like int a = true, +/// and replaces it with integer literals like int a = 1 +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-bool-to-integer-conversion.html +class BoolToIntegerConversionCheck : public ClangTidyCheck { +public: + BoolToIntegerConversionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void changeFunctionReturnType(const FunctionDecl &FD, const Expr &Return); +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_BOOL_TO_INTEGER_CONVERSION_H Index: clang-tidy/bugprone/BoolToIntegerConversionCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/bugprone/BoolToIntegerConversionCheck.cpp @@ -0,0 +1,126 @@ +//===--- BoolToIntegerConversionCheck.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 "BoolToIntegerConversionCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +AST_MATCHER(FieldDecl, isOneBitBitField) { + return Node.isBitField() && + Node.getBitWidthValue(Finder->getASTContext()) == 1; +} + +void BoolToIntegerConversionCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + auto convertingToOneBitBitfieldInInitializer = hasAncestor( + cxxConstructorDecl(hasAnyConstructorInitializer(cxxCtorInitializer( + forField(isOneBitBitField()), + withInitializer(implicitCastExpr(equalsBoundNode("cast"))))))); + + auto convertingToOneBitBitfieldByOperator = + hasParent(binaryOperator(hasLHS(memberExpr( + hasDeclaration(fieldDecl(isOneBitBitField()).bind("bitfield")))))); + + // FIXME: when use "has" matcher instead of hasDescendant when "has" matcher + // will not skip casts. + auto boolOperand = expr(ignoringImpCasts(hasType(booleanType()))); + auto comparingWithBool = hasParent(binaryOperator(hasLHS(boolOperand), hasRHS(boolOperand))); + + + auto isInReturnStmt = hasParent(returnStmt()); + auto forTheSameFunction = forFunction(equalsBoundNode("function")); + auto returnNotBool = returnStmt(forTheSameFunction, unless(has(expr(hasType(booleanType()))))); + auto allReturnsBool = hasBody(unless(hasDescendant(returnNotBool))); + + auto soughtCast = + implicitCastExpr(has(cxxBoolLiteral().bind("bool_literal"))).bind("cast"); + + Finder->addMatcher( + stmt(allOf(soughtCast, + unless(anyOf( + convertingToOneBitBitfieldByOperator, + convertingToOneBitBitfieldInInitializer, comparingWithBool, + allOf(isInReturnStmt, + forFunction(functionDecl().bind("function")), + forFunction(allReturnsBool)))))), + this); + + Finder->addMatcher(decl(functionDecl().bind("function"), + functionDecl(allReturnsBool, + hasDescendant(returnStmt(has(expr().bind("return")), + forTheSameFunction)), + unless(returns(booleanType())))), + this); +} + +void BoolToIntegerConversionCheck::check( + const MatchFinder::MatchResult &Result) { + + if (const auto *Function = Result.Nodes.getNodeAs("function")) { + const auto *Return = Result.Nodes.getNodeAs("return"); + return changeFunctionReturnType(*Function, *Return); + } + + const auto *BoolLiteral = + Result.Nodes.getNodeAs("bool_literal"); + const auto *Cast = Result.Nodes.getNodeAs("cast"); + const auto Type = Cast->getType().getLocalUnqualifiedType(); + auto Location = BoolLiteral->getLocation(); + + auto Diag = diag(Location, "implicitly converting bool literal to %0; use " + "integer literal instead") + << Type; + + if (!Location.isMacroID()) + Diag << FixItHint::CreateReplacement(BoolLiteral->getSourceRange(), + BoolLiteral->getValue() ? "1" : "0"); +} + +void BoolToIntegerConversionCheck::changeFunctionReturnType( + const FunctionDecl &FD, const Expr &Return) { + + diag(FD.getLocation(), + "function has return type %0 but returns only bools") + << FD.getReturnType(); + + auto Diag = diag(Return.getExprLoc(), "converting bool to %0 here", + DiagnosticIDs::Level::Note) + << FD.getReturnType(); + + + if (auto *MD = dyn_cast(&FD)) { + // We skip functions that are virtual, because we would have to be sure that + // all other overriden functions can be changed + if (MD->isVirtual()) + return; + } + + // Abort if there is redeclaration in macro + for (const auto &Redecl : FD.redecls()) + if (Redecl->getLocation().isMacroID()) + return; + + // Change type for all declarations. + for (const auto &Redecl : FD.redecls()) + Diag << FixItHint::CreateReplacement(Redecl->getReturnTypeSourceRange(), + "bool"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: clang-tidy/bugprone/BugProneModule.cpp =================================================================== --- /dev/null +++ clang-tidy/bugprone/BugProneModule.cpp @@ -0,0 +1,40 @@ +//===------- BugProneTidyModule.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 "../ClangTidy.h" +#include "../ClangTidyModule.h" +#include "../ClangTidyModuleRegistry.h" +#include "BoolToIntegerConversionCheck.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +class BugProneModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "bugprone-bool-to-integer-conversion"); + } +}; + +// Register the BoostModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add X("bugprone-module", + "Add bugprone checks."); + +} // namespace bugprone + +// This anchor is used to force the linker to link in the generated object file +// and thus register the BoostModule. +volatile int BugProneModuleAnchorSource = 0; + +} // namespace tidy +} // namespace clang Index: clang-tidy/bugprone/CMakeLists.txt =================================================================== --- /dev/null +++ clang-tidy/bugprone/CMakeLists.txt @@ -0,0 +1,14 @@ +set(LLVM_LINK_COMPONENTS support) + +add_clang_library(clangTidyBugProneModule + BugProneModule.cpp + BoolToIntegerConversionCheck.cpp + + LINK_LIBS + clangAST + clangASTMatchers + clangBasic + clangLex + clangTidy + clangTidyUtils + ) Index: clang-tidy/plugin/CMakeLists.txt =================================================================== --- clang-tidy/plugin/CMakeLists.txt +++ clang-tidy/plugin/CMakeLists.txt @@ -8,6 +8,7 @@ clangFrontend clangSema clangTidy + clangTidyBugProneModule clangTidyBoostModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule Index: clang-tidy/tool/CMakeLists.txt =================================================================== --- clang-tidy/tool/CMakeLists.txt +++ clang-tidy/tool/CMakeLists.txt @@ -10,6 +10,7 @@ clangASTMatchers clangBasic clangTidy + clangTidyBugProneModule clangTidyBoostModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule Index: clang-tidy/tool/ClangTidyMain.cpp =================================================================== --- clang-tidy/tool/ClangTidyMain.cpp +++ clang-tidy/tool/ClangTidyMain.cpp @@ -413,6 +413,11 @@ return 0; } +// This anchor is used to force the linker to link the BugProneModule. +extern volatile int BugProneModuleAnchorSource; +static int LLVM_ATTRIBUTE_UNUSED BugProneModuleAnchorDestination = + BugProneModuleAnchorSource; + // This anchor is used to force the linker to link the CERTModule. extern volatile int CERTModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination = Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -63,6 +63,13 @@ explain them more clearly, and provide more accurate fix-its for the issues identified. The improvements since the 3.8 release include: +- New bugprone module containing checks looking for bugprone code. + +- New `bugprone-bool-to-integer-conversion + `_ check + + Replaces bool literals which are being implicitly cast to integers with integer literals. + - New Boost module containing checks for issues with Boost library. - New `boost-use-to-string Index: docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/bugprone-bool-to-integer-conversion.rst @@ -0,0 +1,55 @@ +.. title:: clang-tidy - bugprone-bool-to-integer-conversion + +bugprone-bool-to-integer-conversion +==================================== + +This check looks for implicit conversion from bool literals to integer types + +.. code-block:: C++ + + int a = false; + vector v(true); // Makes vector of one element + if (a == true) {} + + // Changes it to + int a = 0; + vector v(1); // Makes vector of one element + if (a == 1) {} + +Because bitfields packing are compiler dependent, check treats single-bit +bitfields as bools + +.. code-block:: C++ + + struct BitFields { + BitFields() : notAFlag(true), b(false) {} + unsigned notAFlag : 3; + unsigned flag : 1; + }; + + // Changes to + struct BitFields { + BitFields() : notAFlag(1), b(false) {} + unsigned notAFlag : 3; + unsigned flag : 1; + }; + +It turns out that the common bug is to have function returning only bools but having int as return type. +If check finds case like this then it function return type to bool. + + +.. code-block:: C++ + int fun() { + return true; + return fun(); + bool z; + return z; + } + + // changes it to + bool fun() { + return true; + return fun(); + bool z; + return z; + } Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -6,6 +6,7 @@ .. toctree:: boost-use-to-string + bugprone-bool-to-integer-conversion cert-dcl03-c (redirects to misc-static-assert) cert-dcl50-cpp cert-dcl54-cpp (redirects to misc-new-delete-overloads) Index: docs/clang-tidy/index.rst =================================================================== --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -67,7 +67,9 @@ * Clang static analyzer checks are named starting with ``clang-analyzer-``. -* Checks related to Boost library starts with ``boost-``. +* Checks related to Boost library starts with ``boost-``. + +* The ``bugprone-*`` checks target code that have some potential bugs. Clang diagnostics are treated in a similar way as check diagnostics. Clang diagnostics are displayed by clang-tidy and can be filtered out using @@ -347,6 +349,8 @@ * `C++ Core Guidelines `_ +* potential `bugprone code + `_ * `CERT Secure Coding Standards `_ * `Google Style Guide Index: test/clang-tidy/bugprone-bool-to-integer-conversion.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-bool-to-integer-conversion.cpp @@ -0,0 +1,217 @@ +// RUN: %check_clang_tidy %s bugprone-bool-to-integer-conversion %t + +const int is42Answer = true; +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicitly converting bool literal to 'int'; use integer literal instead [bugprone-bool-to-integer-conversion] +// CHECK-FIXES: const int is42Answer = 1;{{$}} + +volatile int noItsNot = false; +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: implicitly converting bool literal to 'int'; {{..}} +// CHECK-FIXES: volatile int noItsNot = 0;{{$}} +int a = 42; +int az = a; + +long long ll = true; +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: implicitly converting bool literal to 'long long';{{..}} +// CHECK-FIXES: long long ll = 1;{{$}} + +void fun(int) {} +#define ONE true + +// No fixup for macros. +int one = ONE; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: implicitly converting bool literal to 'int'; use integer literal instead [bugprone-bool-to-integer-conversion] + +void test() { + fun(ONE); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: implicitly converting bool{{..}} + + fun(42); + fun(true); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: implicitly {{..}} +// CHECK-FIXES: fun(1);{{$}} +} + +char c = true; +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicitly {{..}} +// CHECK-FIXES: char c = 1; + +float f = false; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: implicitly converting bool literal to 'float';{{..}} +// CHECK-FIXES: float f = 0; + +struct Blah { + Blah(int blah) { } +}; + +const int &ref = false; +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: implicitly converting bool literal to 'int'{{..}} +// CHECK-FIXES: const int &ref = 0; + +Blah bla = true; +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicitly converting bool literal to 'int'{{..}} +// CHECK-FIXES: Blah bla = 1; + +Blah bla2 = 1; + +char c2 = 1; +char c3 = '0'; +bool b = true; + +// Don't warn of bitfields of size 1. Unfortunately we can't just +// change type of flag to bool, because some compilers like MSVC doesn't +// pack bitfields of different types. +struct BitFields { + BitFields() : a(true), flag(false) {} +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: implicitly converting +// CHECK-FIXES: BitFields() : a(1), flag(false) {} + + unsigned a : 3; + unsigned flag : 1; +}; + +struct some_struct { + bool p; + int z; +}; + +void testBitFields() { + BitFields b; + b.flag = true; + b.a = true; +// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicitly converting +// CHECK-FIXES: b.a = 1; + + b.flag |= true; + some_struct s; + s.p |= true; + s.z |= true; +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicitly converting +// CHECK-FIXES: s.z |= 1; +} + +bool returnsBool() { return true; } + +void test_operators() { + bool p = false; + if (p == false) { + + } + int z = 1; + if (z == true) { +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicitly converting +// CHECK-FIXES: if (z == 1) { + + } + bool p2 = false != p; + + int z2 = z - true; +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: implicitly converting +// CHECK-FIXES: int z2 = z - 1; + + bool p3 = z + true; +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: implicitly converting +// CHECK-FIXES: bool p3 = z + 1; + + if(true && p == false) {} + if (returnsBool() == false) {} +} + +bool ok() { + return true; + { + return false; + } + bool z; + return z; +} + +int change_type(); +// CHECK-FIXES: bool change_type(); + +// This function returns only bools +int change_type() { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function has return type 'int' but returns only bools [bugprone-bool-to-integer-conversion] + // CHECK-FIXES: bool change_type() { + + bool p; + return p; + // CHECK-MESSAGES: :[[@LINE-1]]:10: note: converting bool to 'int' here + + return true; + { + return false; + } + return ok(); +} + +int change_type(int); + +char change_type2() { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function has return type 'char' but returns only bools [bugprone-bool-to-integer-conversion] + // CHECK-FIXES: bool change_type2() { + + bool z; + return z; + // CHECK-MESSAGES: :[[@LINE-1]]:10: note: converting bool to 'char' here +} + +int return_int() { + return 2; + { + return true; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicitly converting bool literal + // CHECK-FIXES: return 1; + } + bool p; + return p; +} + +int return_int2() { + { + return true; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicitly converting bool literal + // CHECK-FIXES: return 1; + } + bool p; + return p; + + int z; + return z; +} + +// This checks if it doesn't match inner lambda returns +void yat() { + auto l = [](int p) { + return p == 42; + }; +} + +int yat2() { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function has return type 'int' but returns only bools + // CHECK-FIXES: bool yat2() { + + auto l = []() { + return 42; + }; + + return true; + // CHECK-MESSAGES: :[[@LINE-1]]:10: note: converting bool to 'int' here +} + +struct A { + virtual int fun() = 0; + int fun2() { return 5; }; +}; + +struct B : A { + int fun() { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function has return type 'int' but returns only bools + return true; + // CHECK-MESSAGES: :[[@LINE-1]]:12: note: converting bool to 'int' here + } + + int fun2() { return true; } + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function has return type 'int' but returns only bools + // CHECK-MESSAGES: :[[@LINE-2]]:23: note: converting bool to 'int' here + // CHECK-FIXES: bool fun2() { +};