Index: clang-tidy/misc/AssertSideEffectCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/AssertSideEffectCheck.h @@ -0,0 +1,52 @@ +//===--- AssertSideEffectCheck.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_MISC_ASSERT_SIDE_EFFECT_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSERT_SIDE_EFFECT_CHECK_H + +#include "../ClangTidy.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include + +namespace clang { +namespace tidy { + +/// \brief Finds \c assert() with side effect. +/// +/// The condition of \c assert() is evaluated only in debug builds so a +/// condition with side effect can cause different behaviour in debug / relesase +/// builds. +/// +/// There are two options: +/// - AssertMacros: AssertMacros: A comma-separated list of the names of assert +/// macros to be checked. +/// The names are separated by commas. Do not use other delimiter +/// like whitespace, semicolon. +/// - CheckFunctionCalls: Whether to treat non-const member and non-member +/// functions as they produce side effects. Disabled by default +/// because it can increase the number of false positive warnings. + +class AssertSideEffectCheck : public ClangTidyCheck { +public: + AssertSideEffectCheck(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 bool CheckFunctionCalls; + const std::string RawAssertList; + SmallVector AssertMacros; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSERT_SIDE_EFFECT_CHECK_H Index: clang-tidy/misc/AssertSideEffectCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/AssertSideEffectCheck.cpp @@ -0,0 +1,115 @@ +//===--- AssertSideEffectCheck.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 "AssertSideEffectCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" +#include +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace ast_matchers { + +AST_MATCHER_P(Expr, hasSideEffect, bool, CheckFunctionCalls) { + const Expr *E = &Node; + + if (const auto *Op = dyn_cast(E)) { + UnaryOperator::Opcode OC = Op->getOpcode(); + return OC == UO_PostInc || OC == UO_PostDec || OC == UO_PreInc || + OC == UO_PreDec; + } + + if (const auto *Op = dyn_cast(E)) { + BinaryOperator::Opcode OC = Op->getOpcode(); + return OC == BO_Assign || OC == BO_MulAssign || OC == BO_DivAssign || + OC == BO_RemAssign || OC == BO_AddAssign || OC == BO_SubAssign || + OC == BO_ShlAssign || OC == BO_ShrAssign || OC == BO_AndAssign || + OC == BO_XorAssign || OC == BO_OrAssign; + } + + if (const auto *OpCallExpr = dyn_cast(E)) { + OverloadedOperatorKind OpKind = OpCallExpr->getOperator(); + return OpKind == OO_Equal || OpKind == OO_PlusEqual || + OpKind == OO_MinusEqual || OpKind == OO_StarEqual || + OpKind == OO_SlashEqual || OpKind == OO_AmpEqual || + OpKind == OO_PipeEqual || OpKind == OO_CaretEqual || + OpKind == OO_LessLessEqual || OpKind == OO_GreaterGreaterEqual || + OpKind == OO_PlusPlus || OpKind == OO_MinusMinus || + OpKind == OO_PercentEqual || OpKind == OO_New || + OpKind == OO_Delete || OpKind == OO_Array_New || + OpKind == OO_Array_Delete; + } + + if (const auto *CExpr = dyn_cast(E)) { + bool Result = CheckFunctionCalls; + if (const auto *FuncDecl = CExpr->getDirectCallee()) + if (const auto *MethodDecl = dyn_cast(FuncDecl)) + Result &= !MethodDecl->isConst(); + return Result; + } + + return isa(E) || isa(E) || isa(E); +} + +} // namespace ast_matchers + +namespace tidy { + +AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckFunctionCalls(Options.get("CheckFunctionCalls", false)), + RawAssertList(Options.get("AssertMacros", "assert")) { + StringRef SR = RawAssertList; + SR.split(AssertMacros, ",", -1, false); +} + +// The options are explained in AssertSideEffectCheck.h. +void AssertSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckFunctionCalls", CheckFunctionCalls); + Options.store(Opts, "AssertMacros", RawAssertList); +} + +void AssertSideEffectCheck::registerMatchers(MatchFinder *Finder) { + const auto ConditionWithSideEffect = + hasCondition(hasDescendant(expr(hasSideEffect(CheckFunctionCalls)))); + Finder->addMatcher( + stmt(anyOf(conditionalOperator(ConditionWithSideEffect), + ifStmt(ConditionWithSideEffect))).bind("condStmt"), + this); +} + +void AssertSideEffectCheck::check(const MatchFinder::MatchResult &Result) { + const ASTContext *ASTCtx = Result.Context; + const auto *CondStmt = Result.Nodes.getNodeAs("condStmt"); + const SourceLocation Loc = CondStmt->getLocStart(); + + if (!Loc.isValid() || !Loc.isMacroID()) + return; + + StringRef MacroName = Lexer::getImmediateMacroName( + Loc, ASTCtx->getSourceManager(), ASTCtx->getLangOpts()); + + // Check if this macro is an assert. + if (std::find(AssertMacros.begin(), AssertMacros.end(), MacroName) == + AssertMacros.end()) + return; + + diag(Loc, "found " + MacroName.str() + "() with side effect"); +} + +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -2,11 +2,13 @@ add_clang_library(clangTidyMiscModule ArgumentCommentCheck.cpp + AssertSideEffectCheck.cpp AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversion.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp MiscTidyModule.cpp + StaticAssertCheck.cpp SwappedArgumentsCheck.cpp UndelegatedConstructor.cpp UnusedRAII.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -11,10 +11,12 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "ArgumentCommentCheck.h" +#include "AssertSideEffectCheck.h" #include "AssignOperatorSignatureCheck.h" #include "BoolPointerImplicitConversion.h" #include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" +#include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" #include "UndelegatedConstructor.h" #include "UniqueptrResetRelease.h" @@ -28,6 +30,8 @@ public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck("misc-argument-comment"); + CheckFactories.registerCheck( + "misc-assert-side-effect"); CheckFactories.registerCheck( "misc-assign-operator-signature"); CheckFactories.registerCheck( @@ -36,6 +40,8 @@ "misc-inaccurate-erase"); CheckFactories.registerCheck( "misc-inefficient-algorithm"); + CheckFactories.registerCheck( + "misc-static-assert"); CheckFactories.registerCheck( "misc-swapped-arguments"); CheckFactories.registerCheck( Index: clang-tidy/misc/StaticAssertCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/StaticAssertCheck.h @@ -0,0 +1,39 @@ +//===--- StaticAssertCheck.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_MISC_STATIC_ASSERT_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATIC_ASSERT_CHECK_H + +#include "../ClangTidy.h" +#include "llvm/ADT/StringRef.h" +#include + +namespace clang { +namespace tidy { + +/// \brief Replaces \c assert() with \c static_assert() if the condition is +/// evaluatable at compile time. +/// +/// The condition of \c static_assert() is evaluated at compile time which is +/// safer and more efficient. +class StaticAssertCheck : public ClangTidyCheck { +public: + StaticAssertCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const SourceLocation getLastParenLoc(const ASTContext *ASTCtx, + SourceLocation AssertLoc); +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATIC_ASSERT_CHECK_H Index: clang-tidy/misc/StaticAssertCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/StaticAssertCheck.cpp @@ -0,0 +1,140 @@ +//===--- StaticAssertCheck.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 "StaticAssertCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +StaticAssertCheck::StaticAssertCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +void StaticAssertCheck::registerMatchers(MatchFinder *Finder) { + const auto IsAlwaysFalse = ignoringParenImpCasts( + anyOf(boolLiteral(equals(false)).bind("isAlwaysFalse"), + integerLiteral(equals(0)).bind("isAlwaysFalse"))); + const auto AssertExprRoot = anyOf( + binaryOperator( + hasOperatorName("&&"), + hasEitherOperand(ignoringImpCasts(stringLiteral().bind("assertMSG"))), + anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalse)), anything())) + .bind("assertExprRoot"), + IsAlwaysFalse); + const auto Condition = expr(anyOf( + expr(ignoringParenCasts(anyOf( + AssertExprRoot, + unaryOperator(hasUnaryOperand(ignoringParenCasts(AssertExprRoot)))))), + anything())); + + Finder->addMatcher( + stmt(anyOf(conditionalOperator(hasCondition(Condition.bind("condition"))), + ifStmt(hasCondition(Condition.bind("condition")))), + unless(isInTemplateInstantiation())).bind("condStmt"), + this); +} + +void StaticAssertCheck::check(const MatchFinder::MatchResult &Result) { + const ASTContext *ASTCtx = Result.Context; + const LangOptions &Opts = ASTCtx->getLangOpts(); + const SourceManager &SM = ASTCtx->getSourceManager(); + const auto *CondStmt = Result.Nodes.getNodeAs("condStmt"); + const auto *Condition = Result.Nodes.getNodeAs("condition"); + const auto *IsAlwaysFalse = Result.Nodes.getNodeAs("isAlwaysFalse"); + const auto *AssertMSG = Result.Nodes.getNodeAs("assertMSG"); + const auto *AssertExprRoot = + Result.Nodes.getNodeAs("assertExprRoot"); + const SourceLocation AssertExpansionLoc = CondStmt->getLocStart(); + + if (!Opts.CPlusPlus11 || !AssertExpansionLoc.isValid() || + !AssertExpansionLoc.isMacroID()) + return; + + StringRef MacroName = + Lexer::getImmediateMacroName(AssertExpansionLoc, SM, Opts); + + if (MacroName != "assert" || !Condition->isEvaluatable(*ASTCtx)) + return; + + // False literal is not the result of macro expansion. + if (IsAlwaysFalse && + !SM.getImmediateSpellingLoc(IsAlwaysFalse->getExprLoc()).isMacroID()) + return; + + const SourceLocation AssertLoc = + SM.getImmediateMacroCallerLoc(AssertExpansionLoc); + + SmallVector FixItHints; + SourceLocation LastParenLoc; + if (AssertLoc.isValid() && !AssertLoc.isMacroID() && + (LastParenLoc = getLastParenLoc(ASTCtx, AssertLoc)).isValid()) { + FixItHints.push_back( + FixItHint::CreateReplacement(SourceRange(AssertLoc), "static_assert")); + + std::string StaticAssertMSG = ", \"\""; + if (AssertExprRoot) { + FixItHints.push_back(FixItHint::CreateRemoval( + SourceRange(AssertExprRoot->getOperatorLoc()))); + FixItHints.push_back(FixItHint::CreateRemoval( + SourceRange(AssertMSG->getLocStart(), AssertMSG->getLocEnd()))); + StaticAssertMSG = (Twine(", \"") + AssertMSG->getString() + "\"").str(); + } + + FixItHints.push_back( + FixItHint::CreateInsertion(LastParenLoc, StaticAssertMSG)); + } + + diag(AssertLoc, "found assert() that could be replaced by static_assert()") + << FixItHints; +} + +const SourceLocation +StaticAssertCheck::getLastParenLoc(const ASTContext *ASTCtx, + SourceLocation AssertLoc) { + const LangOptions &Opts = ASTCtx->getLangOpts(); + const SourceManager &SM = ASTCtx->getSourceManager(); + + llvm::MemoryBuffer *Buffer = SM.getBuffer(SM.getFileID(AssertLoc)); + if (!Buffer) + return SourceLocation(); + + const char *BufferPos = SM.getCharacterData(AssertLoc); + + Token Token; + Lexer Lexer(SM.getLocForStartOfFile(SM.getFileID(AssertLoc)), Opts, + Buffer->getBufferStart(), BufferPos, Buffer->getBufferEnd()); + + // assert first left parenthesis + if (Lexer.LexFromRawLexer(Token) || Lexer.LexFromRawLexer(Token) || + !Token.is(tok::l_paren)) + return SourceLocation(); + + unsigned int ParenCount = 1; + while (ParenCount && !Lexer.LexFromRawLexer(Token)) { + if (Token.is(tok::l_paren)) + ++ParenCount; + else if (Token.is(tok::r_paren)) + --ParenCount; + } + + return Token.getLocation(); +} + +} // namespace tidy +} // namespace clang Index: test/clang-tidy/misc-assert-side-effect.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-assert-side-effect.cpp @@ -0,0 +1,92 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-assert-side-effect %t -config="{CheckOptions: [{key: misc-assert-side-effect.CheckFunctionCalls, value: 1}, {key: misc-assert-side-effect.AssertMacros, value: 'assert,my_assert'}]}" -- +// REQUIRES: shell + +//===--- assert definition block ------------------------------------------===// +int abort() { return 0; } + +#ifdef NDEBUG +#define assert(x) 1 +#else +#define assert(x) \ + if (!(x)) \ + (void)abort() +#endif + +#ifdef NDEBUG +#define my_assert(x) 1 +#else +#define my_assert(x) \ + ((void)((x) ? 1 : abort())) +#endif + +#ifdef NDEBUG +#define not_my_assert(x) 1 +#else +#define not_my_assert(x) \ + if (!(x)) \ + (void)abort() +#endif +//===----------------------------------------------------------------------===// + +class MyClass { +public: + bool badFunc(int a, int b) { return a * b > 0; } + bool goodFunc(int a, int b) const { return a * b > 0; } + + MyClass &operator=(const MyClass &rhs) { return *this; } + + int operator-() { return 1; } + + operator bool() const { return true; } + + void operator delete(void *p) {} +}; + +bool freeFunction() { + return true; +} + +int main() { + + int X = 0; + bool B = false; + assert(X == 1); + + assert(X = 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect [misc-assert-side-effect] + my_assert(X = 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found my_assert() with side effect + not_my_assert(X = 1); + + assert(++X); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + assert(!B); + + assert(B || true); + + assert(freeFunction()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + + MyClass mc; + assert(mc.badFunc(0, 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + assert(mc.goodFunc(0, 1)); + + MyClass mc2; + assert(mc2 = mc); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + + assert(-mc > 0); + + MyClass *mcp; + assert(mcp = new MyClass); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + + assert((delete mcp, false)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + + assert((throw 1, false)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() with side effect + + return 0; +} Index: test/clang-tidy/misc-static-assert.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-static-assert.cpp @@ -0,0 +1,80 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-static-assert %t +// REQUIRES: shell + +void abort() {} +#ifdef NDEBUG +#define assert(x) 1 +#else +#define assert(x) \ + if (!(x)) \ + abort() +#endif + +#define ZERO_MACRO 0 + +#define my_macro() assert(0 == 1) +// CHECK-FIXES: #define my_macro() assert(0 == 1) + +constexpr bool myfunc(int a, int b) { return a * b == 0; } + +class A { +public: + bool method() { return true; } +}; + +class B { +public: + constexpr bool method() { return true; } +}; + +template void doSomething(T t) { + assert(myfunc(1, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be replaced by static_assert() [misc-static-assert] + // CHECK-FIXES: {{^ }}static_assert(myfunc(1, 2), ""); + + assert(t.method()); + // CHECK-FIXES: {{^ }}assert(t.method()); +} + +int main() { + my_macro(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be + // CHECK-FIXES: {{^ }}my_macro(); + + assert(myfunc(1, 2) && (3 == 4)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be + // CHECK-FIXES: {{^ }}static_assert(myfunc(1, 2) && (3 == 4), ""); + + int x = 1; + assert(x == 0); + // CHECK-FIXES: {{^ }}assert(x == 0); + + A a; + B b; + + doSomething(a); + doSomething(b); + + assert(false); + // CHECK-FIXES: {{^ }}assert(false); + + assert(ZERO_MACRO); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be + // CHECK-FIXES: {{^ }}static_assert(ZERO_MACRO, ""); + + assert(0 && "Don't report me!"); + // CHECK-FIXES: {{^ }}assert(0 && "Don't report me!"); + + assert(false && "Don't report me!"); + // CHECK-FIXES: {{^ }}assert(false && "Don't report me!"); + + assert(ZERO_MACRO && "Report me!"); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be + // CHECK-FIXES: {{^ }}static_assert(ZERO_MACRO , "Report me!"); + + assert(10==5 && "Report me!"); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be + // CHECK-FIXES: {{^ }}static_assert(10==5 , "Report me!"); + + return 0; +}