Fix clang-tidy patterns to adapt to newly added ExprWithCleanups nodes.

[timshen]

Summary

This is a fix for the new ExprWithCleanups introduced by clang's temporary variable lifetime marks change.

[alexfh]

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

[alexfh]

In D21243#455999, @alexfh wrote:

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

[timshen]

In D21243#455999, @alexfh wrote:

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests?

This just fixes the tests broken by D20498. I didn't look at other patterns.

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

A MaterializeTemporaryExpr now will always generate an ExprWithCleanups at full-expr entry. That is to say, there must be an ExprWithCleanups as an ancestor node of a MaterializeTemporaryExpr.

For example, the previous pattern:

fooStmt(has(
  barExpr(
    hasDescendant(
      materializeTemporaryExpr()))))

should be changed to:

fooStmt(has(
  exprWithCleanups(
    barExpr(
      hasDescendant(
        materializeTemporaryExpr())))

There are patterns around that are not explicitly checking for a MaterializeTemporaryExpr. If in the checked AST there is a MaterializeTemporaryExpr, an ignoringExprWithCleanups should be added to each of these patterns.

Does this help?

[timshen]

Updated the patch to use ignorngImplicit/Expr::IgnoreImplicit matcher.

I think there is a systematic way to do this change:
Look at every ignoringImpCasts, ignoringParenCasts, ignoringParenImpCasts, ignoringParens, and
Expr::IgnoreImpCasts, Expr::IgnoreParenCasts, Expr::IgnoreParenImpCasts, Expr::IgnoreParens to
see if they can be changed to ignoringImplicit or Expr::IgnoreImplicit (which also ignores
ExprWithCleanups).

I'd say for most of the cases ExprWithCleanups should be ignored, but I'm not sure if I'm the
right person to look at these patterns.

[alexfh]

Yes, you're probably not the right person to fix our test coverage. Thanks for fixing the stuff that broke and I hope we'll figure out what else needs to be fixed in a finite time after the change is committed ;)

LG

clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp

void TwineLocalCheck::check(const MatchFinder::MatchResult &Result) {
   const VarDecl *VD = Result.Nodes.getNodeAs<VarDecl>("variable");
   auto Diag = diag(VD->getLocation(),
                    "twine variables are prone to use-after-free bugs");
 
   // If this VarDecl has an initializer try to fix it.
   if (VD->hasInit()) {
     // Peel away implicit constructors and casts so we can see the actual type
     // of the initializer.
-    const Expr *C = VD->getInit();
+    const Expr *C = VD->getInit()->IgnoreImplicit();
+
     while (isa<CXXConstructExpr>(C))
       C = cast<CXXConstructExpr>(C)->getArg(0)->IgnoreParenImpCasts();
 
     SourceRange TypeRange =
         VD->getTypeSourceInfo()->getTypeLoc().getSourceRange();
 
     // A real Twine, turn it into a std::string.
     if (VD->getType()->getCanonicalTypeUnqualified() ==

clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp

 //===--- DanglingHandleCheck.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 "DanglingHandleCheck.h"
+#include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
+using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 
 ast_matchers::internal::BindableMatcher<Stmt>

 void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
   // Return a local.
   Finder->addMatcher(
       returnStmt(
           // The AST contains two constructor calls:
           //   1. Value to Handle conversion.
           //   2. Handle copy construction.
           // We have to match both.
-          has(ignoringParenImpCasts(handleFrom(
+          has(ignoringImplicit(handleFrom(
               IsAHandle,
               handleFrom(IsAHandle, declRefExpr(to(varDecl(
                                         // Is function scope ...
                                         hasAutomaticStorageDuration(),
                                         // ... and it is a local array or Value.
                                         anyOf(hasType(arrayType()),
                                               hasType(recordDecl(
                                                   unless(IsAHandle))))))))))),

clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp

 //===--- LoopConvertCheck.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 "LoopConvertCheck.h"
+#include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 using namespace llvm;
 
 namespace clang {
StatementMatcher IteratorBoundMatcher =
                      declRefExpr(to(varDecl().bind(ConditionEndVarName)))),
                  ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
                  materializeTemporaryExpr(ignoringParenImpCasts(
                      expr(EndCallMatcher).bind(EndCallName)))));
 
   StatementMatcher IteratorComparisonMatcher = expr(
       ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ConditionVarName)))));
 
-  StatementMatcher OverloadedNEQMatcher =
+  auto OverloadedNEQMatcher = matchers::ignoringImplicit(
       cxxOperatorCallExpr(hasOverloadedOperatorName("!="), argumentCountIs(2),
                           hasArgument(0, IteratorComparisonMatcher),
-                          hasArgument(1, IteratorBoundMatcher));
+                          hasArgument(1, IteratorBoundMatcher)));
 
   // This matcher tests that a declaration is a CXXRecordDecl that has an
   // overloaded operator*(). If the operator*() returns by value instead of by
   // reference then the return type is tagged with DerefByValueResultName.
   internal::Matcher<VarDecl> TestDerefReturnsByValue =
       hasType(cxxRecordDecl(hasMethod(allOf(
           hasOverloadedOperatorName("*"),
           anyOf(

clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp

 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector<int>::iterator it;
 ///   vector<int>::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
 const Expr *digThroughConstructors(const Expr *E) {
   if (!E)
     return nullptr;
-  E = E->IgnoreParenImpCasts();
+  E = E->IgnoreImplicit();
   if (const auto *ConstructExpr = dyn_cast<CXXConstructExpr>(E)) {
     // The initial constructor must take exactly one parameter, but base class
     // and deferred constructors can take more.
     if (ConstructExpr->getNumArgs() != 1 ||
         ConstructExpr->getConstructionKind() != CXXConstructExpr::CK_Complete)
       return nullptr;
     E = ConstructExpr->getArg(0);
     if (const auto *Temp = dyn_cast<MaterializeTemporaryExpr>(E))

clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp

 ///
 /// varDecl(hasWrittenNonListInitializer()) maches \c I and \c A but not \c B
 /// or \c C.
 AST_MATCHER(VarDecl, hasWrittenNonListInitializer) {
   const Expr *Init = Node.getAnyInitializer();
   if (!Init)
     return false;
 
+  Init = Init->IgnoreImplicit();
+
   // The following test is based on DeclPrinter::VisitVarDecl() to find if an
   // initializer is implicit or not.
   if (const auto *Construct = dyn_cast<CXXConstructExpr>(Init)) {
     return !Construct->isListInitialization() && Construct->getNumArgs() > 0 &&
            !Construct->getArg(0)->isDefaultArgument();
   }
   return Node.getInitStyle() != VarDecl::ListInit;
 }

clang-tools-extra/trunk/clang-tidy/readability/RedundantStringInitCheck.cpp

void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
 
   // Match a string constructor expression with an empty string literal.
   const auto EmptyStringCtorExpr =
       cxxConstructExpr(StringConstructorExpr,
           hasArgument(0, ignoringParenImpCasts(
                              stringLiteral(hasSize(0)))));
 
   const auto EmptyStringCtorExprWithTemporaries =
-      expr(ignoringImplicit(
-          cxxConstructExpr(StringConstructorExpr,
-              hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)))));
+                       hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)));
 
   // Match a variable declaration with an empty string literal as initializer.
   // Examples:
   //     string foo = "";
   //     string bar("");
   Finder->addMatcher(
-      namedDecl(varDecl(hasType(cxxRecordDecl(hasName("basic_string"))),
-                        hasInitializer(
-                            expr(anyOf(EmptyStringCtorExpr,
-                                       EmptyStringCtorExprWithTemporaries))
+      namedDecl(
+          varDecl(hasType(cxxRecordDecl(hasName("basic_string"))),
+                  hasInitializer(expr(ignoringImplicit(anyOf(
+                                          EmptyStringCtorExpr,
+                                          EmptyStringCtorExprWithTemporaries)))
                                      .bind("expr"))),
-                unless(parmVarDecl()))
           .bind("decl"),
       this);
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CtorExpr = Result.Nodes.getNodeAs<Expr>("expr");
   const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl");
   diag(CtorExpr->getExprLoc(), "redundant string initialization")
       << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
                                       Decl->getName());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang