Index: clang-tidy/misc/AssertSideEffectCheck.h =================================================================== --- clang-tidy/misc/AssertSideEffectCheck.h +++ clang-tidy/misc/AssertSideEffectCheck.h @@ -0,0 +1,40 @@ +//===--- 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 conition of \c assert() is evaluated only in debug builds so a condition +/// with side effect can cause different behaviour in debug / relesase builds. +class AssertSideEffectCheck : public ClangTidyCheck { + const bool CheckFunctionCalls; + const std::string RawAssertList; + llvm::SmallVector AssertList; + +public: + AssertSideEffectCheck(llvm::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; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSERT_SIDE_EFFECT_CHECK_H Index: clang-tidy/misc/AssertSideEffectCheck.cpp =================================================================== --- clang-tidy/misc/AssertSideEffectCheck.cpp +++ clang-tidy/misc/AssertSideEffectCheck.cpp @@ -0,0 +1,114 @@ +//===--- 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 = llvm::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 = llvm::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 *CExpr = llvm::dyn_cast(E)) { + const auto *OpCallExpr = llvm::dyn_cast(E); + if (OpCallExpr) { + OverloadedOperatorKind OpKind = OpCallExpr->getOperator(); + return OpKind == OO_Equal || OpKind == OO_PlusEqual || + OpKind == OO_MinusEqual || OpKind == OO_StarEqual || + OpKind == OO_SlashEqual || OpKind == OO_SlashEqual || + OpKind == OO_PipeEqual || OpKind == OO_CaretEqual || + OpKind == OO_LessLessEqual || OpKind == OO_GreaterGreaterEqual || + OpKind == OO_PlusPlus || OpKind == OO_MinusMinus || + OpKind == OO_PercentEqual; + } + + bool RetVal = CheckFunctionCalls; + if (const auto *FuncDecl = CExpr->getDirectCallee()) + if (const auto *MethDecl = llvm::dyn_cast(FuncDecl)) + RetVal &= !MethDecl->isConst(); + return RetVal; + } + + return llvm::isa(E) || llvm::isa(E) || + llvm::isa(E); +} + +} // namespace ast_matchers + +namespace tidy { + +AssertSideEffectCheck::AssertSideEffectCheck(llvm::StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckFunctionCalls(Options.get("CheckFunctionCalls", false)), + RawAssertList(Options.get("AssertList", "assert")) { + llvm::StringRef SR = RawAssertList; + SR.split(AssertList, " ", -1, false); +} + +void AssertSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckFunctionCalls", CheckFunctionCalls); + Options.store(Opts, "AssertList", 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) { + ASTContext *const ASTCtx = Result.Context; + const auto *CondStmt = Result.Nodes.getNodeAs("condStmt"); + const SourceLocation Loc = CondStmt->getLocStart(); + + if (!Loc.isValid() || !Loc.isMacroID()) + return; + + llvm::StringRef MacroName = Lexer::getImmediateMacroName( + Loc, ASTCtx->getSourceManager(), ASTCtx->getLangOpts()); + + // Check if this macro is an assert. + if (std::find(AssertList.begin(), AssertList.end(), MacroName) == + AssertList.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 =================================================================== --- clang-tidy/misc/StaticAssertCheck.h +++ clang-tidy/misc/StaticAssertCheck.h @@ -0,0 +1,35 @@ +//===--- 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(llvm::StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATIC_ASSERT_CHECK_H Index: clang-tidy/misc/StaticAssertCheck.cpp =================================================================== --- clang-tidy/misc/StaticAssertCheck.cpp +++ clang-tidy/misc/StaticAssertCheck.cpp @@ -0,0 +1,128 @@ +//===--- 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/StringRef.h" +#include "llvm/Support/Casting.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +StaticAssertCheck::StaticAssertCheck(llvm::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) { + ASTContext *const 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; + + llvm::StringRef MacroName = Lexer::getImmediateMacroName( + AssertExpansionLoc, ASTCtx->getSourceManager(), 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); + + FixItHint NameHint, AssertExprRootHint, SLitHint, ArgHint; + if (AssertLoc.isValid() && !AssertLoc.isMacroID()) { + NameHint = + FixItHint::CreateReplacement(SourceRange(AssertLoc), "static_assert"); + + std::string StaticAssertMSG = ", \"\""; + if (AssertExprRoot) { + AssertExprRootHint = FixItHint::CreateRemoval( + SourceRange(AssertExprRoot->getOperatorLoc())); + SLitHint = FixItHint::CreateRemoval( + SourceRange(AssertMSG->getLocStart(), AssertMSG->getLocEnd())); + StaticAssertMSG = + (llvm::Twine(", \"") + AssertMSG->getString() + "\"").str(); + } + + llvm::MemoryBuffer *Buffer = SM.getBuffer(SM.getFileID(AssertLoc)); + if (!Buffer) + return; + + const char *BufferPos = SM.getCharacterData(AssertLoc); + + Token Token; + Lexer Lexer(SM.getLocForStartOfFile(SM.getFileID(AssertLoc)), Opts, + Buffer->getBufferStart(), BufferPos, Buffer->getBufferEnd()); + Lexer.SetCommentRetentionState(false); + + while (!Lexer.LexFromRawLexer(Token) && Token.isNot(tok::l_paren)) + ; + + unsigned int ParenCount = 1; + while (ParenCount && !Lexer.LexFromRawLexer(Token)) { + if (Token.is(tok::l_paren)) + ++ParenCount; + if (Token.is(tok::r_paren)) + --ParenCount; + } + + ArgHint = FixItHint::CreateInsertion(Token.getLocation(), StaticAssertMSG); + } + + diag(AssertLoc, "found assert() that could be replaced by static_assert()") + << NameHint << AssertExprRootHint << SLitHint << ArgHint; +} + +} // namespace tidy +} // namespace clang Index: test/clang-tidy/misc-assert-side-effect.cpp =================================================================== --- test/clang-tidy/misc-assert-side-effect.cpp +++ test/clang-tidy/misc-assert-side-effect.cpp @@ -0,0 +1,63 @@ + +// 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.AssertList, 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; } + + operator bool() const { return true; } +}; + +int main() { + + int X = 0; + 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); + + 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 + + return 0; +} Index: test/clang-tidy/misc-static-assert.cpp =================================================================== --- test/clang-tidy/misc-static-assert.cpp +++ 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; +}