[clang-tidy] Assert related checkers

[eszasip]

Summary

This patch contains two assert related checkers. These checkers are the part of those that is being open sourced by Ericsson (http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-December/040520.html).

The checkers:

AssertSideEffect:
/ \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.

StaticAssert:
/ \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.

[alexfh]

Nice checks! Could you run it over LLVM/Clang and see whether they finds something?

Also, a couple of comments below.

clang-tidy/misc/AssertSideEffectCheck.cpp

22

This one is not needed, as the rest of the file is inside namespace clang {}. Let's just leave using namespace clang::ast_matchers; here.

34

AFAIU, RetVal can never be changed below. So why not just return here? Same is valid for other cases.

92

Does it work if you pull out the common part:

auto ConditionWithSideEffect = 
  hasCondition(hasDescendant(expr(hasSideEffect(CheckFunctionCalls)))));
Finder->addMatcher(
      stmt(anyOf(conditionalOperator(ConditionWithSideEffect), ifStmt(ConditionWithSideEffect))));

?

110

nit: Please finish statements with a period It makes comments look nicer

test/clang-tidy/misc-assert-side-effect.cpp

12

Can you add a conditional operator-based implementation for testing?

[eszasip]

Fixed style issues. Added a conditional operator-based assert() to misc-assert-side-effect.cpp.

StaticAssertCheck:
-added new FixIt cases: assert((...) && "AssertMsg") --> static_assert((...), "AssertMsg")
-added test cases for each FixIt case

[alexfh]

Thanks for addressing the comments. A few comments more below and now I managed to review the second check as well.

clang-tidy/misc/AssertSideEffectCheck.cpp

45

1. Why do you need to place this inside the if (const auto *CExpr = llvm::dyn_cast<CallExpr>(E)) {?

2. I'd prefer to put the definition inside the condition:
   
   if (const auto *OpCallExpr = llvm::dyn_cast<CXXOperatorCallExpr>(E)) { ...

50

Two comparisons with OO_SlashEqual.

57

Maybe s/RetVal/Result/? It's the same length, but it's a word.

59

MethodDecl is just two letters more, but reads better.

64

Here and elsewhere: I think, dyn_cast, isa and StringRef are imported in either clang or global namespace. Please check if the code compiles without explicit qualifiers.

75

Why is this option needed? Why "false" is the right default value?

76

The fact that it's a list can be conveyed by just using a plural, e.g. AssertMacros.

78

It seems that comma-separated lists are more common.

clang-tidy/misc/StaticAssertCheck.cpp

71

s/ASTCtx->getSourceManager()/SM/

84

I'd replace these with a SmallVector<FixItHint, 4>.

91

I think, in case there's a message, the fixes should be made as follows:

• insert a comma right after the end of the LHS (matters if there is a line break or a comment after the LHS)
• remove the "&&" and any whitespace after it
• don't touch the message at all.

In case there is no message originally, just insert a ", \"\"" right before the closing brace.

WDYT?

108

Why do you care about retention of comments? Does it matter?

110

Do you expect to meet anything between "assert" and "("? Maybe just use Lexer::getLocForEndOfToken and start the search of parentheses from after the "assert" token?

114

Please pull the search of matching parentheses in a function. Probably, together with the Lexer, Buffer and Token variables.

117

nit: I'd use else if to emphasize that these cases are mutually exclusive.

clang-tidy/misc/StaticAssertCheck.h

2

It's fine for now, but generally I'd prefer if you sent one patch per check. It would be easier to review and the review of each patch wouldn't block the other one.

Thanks!

[LegalizeAdulthood]

clang-tidy/misc/AssertSideEffectCheck.h

24

condition misspelled

26–31

I always find it odd when reading a class to be shown its internal details before I'm shown its public API. I guess it saves having to write a single line private:, but it makes it more distracting to read the source code.

[LegalizeAdulthood]

clang-tidy/misc/AssertSideEffectCheck.cpp

47–54

What about OO_AmpEqual?

test/clang-tidy/misc-assert-side-effect.cpp

43–63

Most of the items detected have no test cases. I used the FileCheck tests to push my implementation forward ensuring that I always had some test case for all the combinations. The test cases can also serve as good executable documentation for everything the check does.

[eszasip]

Added:
--more test cases to AssertSideEffectChecker
--comments explaining the options of AssertSideEffectChecker

Fixed everything that alexfh and LegalizeAdulthood suggested except the "whitespace before the comma" problem.

clang-tidy/misc/AssertSideEffectCheck.cpp

47–54

That's what I wanted to write instead of the second OO_SlashEqual.

clang-tidy/misc/StaticAssertCheck.cpp

91

I also thought about this solution but it would generate incorrect source code if the argument of assert was in parentheses or "&&" had its operands swapped. These cases might be rare but my main aim was not to generate incorrect code.

About fixing the 'comma problem' while keeping the genericness:
I could have ignored the second case or written an even bigger matcher (and FixIt).
In the first case, distinguishing between parentheses in the argument or in the macro definition and deciding where the remaining whitespaces belong would make the code so complicated that I thought it didn't worth it.

If you suggest generating a nice-looking source code and risking that it will not always complie, I will change that part.

[alexfh]

Sorry for the long delay. This looks better, but still a few comments.

clang-tidy/misc/AssertSideEffectCheck.cpp

45

So why do you need to place this if inside the if (.. = dyn_cast<CallExpr>(E))?

clang-tidy/misc/AssertSideEffectCheck.h

25

nit: The comment is now mis-formatted.

29

How about making the comment a bit less wordy:

• AssertMacros: A comma-separated list of the names of assert macros to be checked.

?

33

This doesn't explain much. "Checking function calls" is not what this option controls. AFAIU, it makes the check treat non-const member calls as though they produce side effects.

I also don't understand what do you mean by "reporting free functions". Maybe add a motivating example?

clang-tidy/misc/StaticAssertCheck.cpp

84

Creating an empty vector and then using push_back() to add whatever fixes you need looks like a more elegant solution.

104

Starting from r230495, you can insert FixItHints directly.

108

As I probably mentioned before, top-level constness doesn't make sense in function parameter types. It is just an implementation detail that is not interesting to the API users, but it makes the interface less readable.

test/clang-tidy/misc-assert-side-effect.cpp

2

Please remove the empty line.

[eszasip]

Fixed the problems alexfh pointed out.

[alexfh]

clang-tidy/misc/StaticAssertCheck.cpp

55

nit: Do you need ASTCtx to point to a non-const ASTContext?

73

I wonder if it's intentional that the other check has an option for assert macro names and this one doesn't.

109

Do you need ASTCtx to point to a non-const ASTContext? I only dislike top-level const ;)

Also, please remove the "const" on the second parameter.

test/clang-tidy/misc-assert-side-effect.cpp

46

This comment doesn't seem to be very useful.

[xazax.hun]

clang-tidy/misc/StaticAssertCheck.cpp

73

I think the fixit might not be valid for a custom assert. Once custom assert macros are supported either those should not be rewritten or it should be configurable whether to rewrite custom macros or not. Or we can just rely on that the users won't run this checker with custom asserts configured and apply fixits mode. What do you think?

[alexfh]

clang-tidy/misc/StaticAssertCheck.cpp

73

Makes sense, the fixits may be incorrect for custom asserts. One possible example is:

CHECK(some_boolean_expression) << "Error message: " << some_parameter;

Another couple of possible solutions:

• configure a separate list of macros that the check should not issue fixits for;
• try to detect automatically whether we can handle the specific construct (e.g. if it's in the form of macro(expression);).

In any case it looks like it's better to do this in a separate patch.

[eszasip]

This comment was removed by eszasip.

[eszasip]

Moved const qualifier to the right place.

[alexfh]

Looks good! (with one nit, see the comment)

Thanks for the contribution!

Do you need me to commit the patch for you?

clang-tidy/misc/AssertSideEffectCheck.cpp

97

nit: Remove the space after the star.

[eszasip]

Removed the space after the star.

Please commit this patch for me.

Thanks for your help so far.

[alexfh]

Committed revision 230943.

A few changes before commit:

• Fixed a couple of issues I noticed (see the comments).
• Removed some top-level consts for consistency with the rest of clang-tidy code (and other reasons I mentioned earlier in reviews).

Thank you for contributing these checks!

clang-tidy/misc/AssertSideEffectCheck.cpp

76

nit: This can be a temporary.

clang-tidy/misc/AssertSideEffectCheck.h

30

This part of the comment is redundant since we already said that this is a comma-separated list.

[alexfh]

I found one issue with the misc-static-assert check. The check warns on assert(false) in some configurations. I've filed http://llvm.org/PR22880.

Could you take a look at the issue?

[lebedev.ri]

test/clang-tidy/misc-assert-side-effect.cpp

67

Is it intentional that the check also warns on free functions even if they are marked as const, .e.g:

cat >test.cpp <<EOL

#include <cassert>

// only works for positive values and zero
template <typename T>
inline constexpr bool __attribute__((const))
isPowerOfTwo(T val) {
  return (val & (~val+1)) == val;
}

int main()
{
  assert(isPowerOfTwo(2 << 2));
}

EOL

Run:

$ clang-tidy -checks misc-assert-side-effect -config="{Checks: 'misc-assert-side-effect', CheckOptions: [{key: 'misc-assert-side-effect.CheckFunctionCalls', value: 1}]}" test.cpp -- -std=c++11
/tmp/test.cpp:14:3: warning: found assert() with side effect [misc-assert-side-effect]
  assert(isPowerOfTwo(2 << 2));
  ^
/usr/include/assert.h:89:4: note: expanded from macro 'assert'
  ((expr)                                                               \
   ^

Am i missing something obvious?

[lebedev.ri]

test/clang-tidy/misc-static-assert.cpp

71

Hmm. Are you sure about this?
I'm having false-positives (as far i'm concerned at least) due to this.

In particular with <sanitizer/asan_interface.h>. If the build is with asan,
then i can include the header and use e.g. __asan_region_is_poisoned()
But if it is not, i can't do that. So i need to wrap it into a macro, which expands
to __asan_region_is_poisoned() if the build is with asan, and to some constant
(e.g. 0) otherwise. And the check, naturally, does not like this.

As a minimal reproducer:

#ifndef MYDEFINE
#define mycheck(x) 0
#define notmycheck(x) 1
#else
int __mycheck(int x);
int __notmycheck(int x);
#define mycheck(x) __mycheck(x)
#define notmycheck(x) __notmycheck(x)
#endif

void f(int x) {
  assert(!mycheck(x));
  assert(!notmycheck(x));
  assert(mycheck(x));
  assert(notmycheck(x));
}

I'd expect the check to not warn regardless of MYDEFINE presence.

clang-tidy/misc/AssertSideEffectCheck.h

+//===--- 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 <string>
+
+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<StringRef, 5> AssertMacros;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSERT_SIDE_EFFECT_CHECK_H

clang-tidy/misc/AssertSideEffectCheck.cpp

+//===--- 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 <algorithm>
+#include <string>
+
+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<UnaryOperator>(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<BinaryOperator>(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<CXXOperatorCallExpr>(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<CallExpr>(E)) {
+    bool Result = CheckFunctionCalls;
+    if (const auto *FuncDecl = CExpr->getDirectCallee())
+      if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
+        Result &= !MethodDecl->isConst();
+    return Result;
+  }
+
+  return isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(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<Stmt>("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

clang-tidy/misc/CMakeLists.txt

 
 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

clang-tidy/misc/MiscTidyModule.cpp

 #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"

 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
+    CheckFactories.registerCheck<AssertSideEffectCheck>(
+        "misc-assert-side-effect");
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversion>(

         "misc-inaccurate-erase");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
+    CheckFactories.registerCheck<StaticAssertCheck>(
+        "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(

clang-tidy/misc/StaticAssertCheck.h

+//===--- 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 <string>
+
+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

clang-tidy/misc/StaticAssertCheck.cpp

+//===--- 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 <string>
+
+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<Stmt>("condStmt");
+  const auto *Condition = Result.Nodes.getNodeAs<Expr>("condition");
+  const auto *IsAlwaysFalse = Result.Nodes.getNodeAs<Expr>("isAlwaysFalse");
+  const auto *AssertMSG = Result.Nodes.getNodeAs<StringLiteral>("assertMSG");
+  const auto *AssertExprRoot =
+      Result.Nodes.getNodeAs<BinaryOperator>("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<FixItHint, 4> 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

test/clang-tidy/misc-assert-side-effect.cpp

+// 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;
+}

test/clang-tidy/misc-static-assert.cpp

+// 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 <class T> 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>(a);
+  doSomething<B>(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;
+}