[clang-tidy] Simplify static assert check

[njames93]

LG with a few nits.

clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp

49–52

Can this be cleaned up to use optionally matcher.

59–61

Can't you use the mapAnyOf matcher here?

[lebedev.ri]

Reverted in be6b9e8ae71768d2e09ec14619ca4ecfdef553fa - https://godbolt.org/z/3zdqvbfxj
While there, please ensure that other such simplifications don't have similar lingering effects.

[aaron.ballman]

In D96223#2788681, @lebedev.ri wrote:

Reverted in be6b9e8ae71768d2e09ec14619ca4ecfdef553fa - https://godbolt.org/z/3zdqvbfxj
While there, please ensure that other such simplifications don't have similar lingering effects.

Thanks for catching the issue!

FWIW, I think that reverting something several months after it lands is a bit disruptive and the test case causing the revert feels arbitrary without further context that should have been explicitly mentioned in the reverting commit message. The context is that this change broke a test that should have caught the failure: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/misc-static-assert.cpp#L86-L87 which led to this bug report: https://bugs.llvm.org/show_bug.cgi?id=50532. I'm still not convinced the correct action was to revert a change with no discussion that already was shipped in a release, however. Next time, I think it'd be a bit friendlier to mention the regression on the patch to see if the author can do a quick fix -- if no one noticed it for almost four months, it's hard to argue it's a critical issue that needs an immediate revert. (The reversion mildly worries me because this change already shipped as part of Clang 12 and larger reversions make for harder git blame logic to follow along with, both of which can lead to potential confusion for folks.)

[xbolva00]

In D96223#2788712, @aaron.ballman wrote:

In D96223#2788681, @lebedev.ri wrote:

Reverted in be6b9e8ae71768d2e09ec14619ca4ecfdef553fa - https://godbolt.org/z/3zdqvbfxj
While there, please ensure that other such simplifications don't have similar lingering effects.

Thanks for catching the issue!

FWIW, I think that reverting something several months after it lands is a bit disruptive and the test case causing the revert feels arbitrary without further context that should have been explicitly mentioned in the reverting commit message. The context is that this change broke a test that should have caught the failure: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/misc-static-assert.cpp#L86-L87 which led to this bug report: https://bugs.llvm.org/show_bug.cgi?id=50532. I'm still not convinced the correct action was to revert a change with no discussion that already was shipped in a release, however. Next time, I think it'd be a bit friendlier to mention the regression on the patch to see if the author can do a quick fix -- if no one noticed it for almost four months, it's hard to argue it's a critical issue that needs an immediate revert. (The reversion mildly worries me because this change already shipped as part of Clang 12 and larger reversions make for harder git blame logic to follow along with, both of which can lead to potential confusion for folks.)

And LLVM policy agrees with you.

Reverts should be reasonably timely. A change submitted two hours ago can be reverted without prior discussion. A change submitted two years ago should not be. Where exactly the transition point is is hard to say, but it’s probably in the handful of days in tree territory. If you are unsure, we encourage you to reply to the commit thread, give the author a bit to respond, and then proceed with the revert if the author doesn’t seem to be actively responding.

https://llvm.org/docs/DeveloperPolicy.html

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang-tools-extra/clang-tidy/misc/StaticAssertCheck.h

 class StaticAssertCheck : public ClangTidyCheck {
 public:
   StaticAssertCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11 || LangOpts.C11;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   SourceLocation getLastParenLoc(const ASTContext *ASTCtx,
                                  SourceLocation AssertLoc);
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICASSERTCHECK_H

clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp

 namespace clang {
 namespace tidy {
 namespace misc {
 
 StaticAssertCheck::StaticAssertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {}
 
 void StaticAssertCheck::registerMatchers(MatchFinder *Finder) {
-  auto NegatedString = unaryOperator(
-      hasOperatorName("!"), hasUnaryOperand(ignoringImpCasts(stringLiteral())));
+  auto NegatedString =
+      unaryOperator(hasOperatorName("!"), hasUnaryOperand(stringLiteral()));
   auto IsAlwaysFalse =
       expr(anyOf(cxxBoolLiteral(equals(false)), integerLiteral(equals(0)),
                  cxxNullPtrLiteralExpr(), gnuNullExpr(), NegatedString))
           .bind("isAlwaysFalse");
-  auto IsAlwaysFalseWithCast = ignoringParenImpCasts(anyOf(
-      IsAlwaysFalse, cStyleCastExpr(has(ignoringParenImpCasts(IsAlwaysFalse)))
-                         .bind("castExpr")));
-  auto AssertExprRoot = anyOf(
-      binaryOperator(
+  auto IsAlwaysFalseWithCast =
+      anyOf(IsAlwaysFalse, cStyleCastExpr(has(IsAlwaysFalse)).bind("castExpr"));
+  auto AssertExprRoot =
+      anyOf(binaryOperator(
                 hasAnyOperatorName("&&", "=="),
-          hasEitherOperand(ignoringImpCasts(stringLiteral().bind("assertMSG"))),
+                hasEitherOperand(stringLiteral().bind("assertMSG")),
                 anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
                       anything()))
                 .bind("assertExprRoot"),
             IsAlwaysFalse);
   auto NonConstexprFunctionCall =
       callExpr(hasDeclaration(functionDecl(unless(isConstexpr()))));
   auto AssertCondition =
-      expr(
-          anyOf(expr(ignoringParenCasts(anyOf(
-                    AssertExprRoot, unaryOperator(hasUnaryOperand(
-                                        ignoringParenCasts(AssertExprRoot)))))),
-                anything()),
+      expr(optionally(expr(anyOf(AssertExprRoot,
+                            unaryOperator(hasUnaryOperand(AssertExprRoot))))),
            unless(findAll(NonConstexprFunctionCall)))
           .bind("condition");
   auto Condition =
-      anyOf(ignoringParenImpCasts(callExpr(
-                hasDeclaration(functionDecl(hasName("__builtin_expect"))),
-                hasArgument(0, AssertCondition))),
+      anyOf(callExpr(traverse(TK_AsIs, callExpr(hasDeclaration(functionDecl(
+                                           hasName("__builtin_expect"))))),
+                     hasArgument(0, AssertCondition)),
             AssertCondition);
 
-  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
-                                         unless(isInTemplateInstantiation()))
-                         .bind("condStmt"),
-                     this);
-
   Finder->addMatcher(
-      ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
-          .bind("condStmt"),
-      this);
+      mapAnyOf(ifStmt, conditionalOperator).with(hasCondition(Condition)).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");