[clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

[ymandel]

Summary

The ExprWithCleanups node is added to the AST along with the elidable
CXXConstructExpr. If it is the outermost node of the node being matched, ignore
it as well.

[gribozavr]

clang/include/clang/ASTMatchers/ASTMatchers.h

6538 ↗

(On Diff #214123)

while you're editing it: "elidable copy constructors"

6540 ↗

(On Diff #214123)

"elide these differences" => "abstract over these differences"?

6541 ↗

(On Diff #214123)

"This matcher skips elidable constructor call AST nodes, ... "?

6556 ↗

(On Diff #214123)

Could you add this matcher as a test?

[ymandel]

Added test; adjusted comments.

[ymandel]

Thanks for the comments. I struggled with the wording so your edits are appreciated.

clang/include/clang/ASTMatchers/ASTMatchers.h

6556 ↗

(On Diff #214123)

Done

Also, I noticed that the matcher is classified as "Traversal" yet its tests are in ASTMatchersNarrowingTest.cpp. Any objection to my moving the tests to ASTMatchersTraversalTest.cpp as a followup patch?

[gribozavr]

clang/include/clang/ASTMatchers/ASTMatchers.h

6556 ↗

(On Diff #214123)

That would make a lot of sense, thank you!

[ymandel]

updated HTML docs.

cfe/trunk/docs/LibASTMatchersReference.html

  void y(X &x) { x; X z; }
  typedef int U;
  class Y { friend class X; };
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt;</td><td class="name" onclick="toggle('ignoringElidableConstructorCall0')"><a name="ignoringElidableConstructorCall0Anchor">ignoringElidableConstructorCall</a></td><td>ast_matchers::Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="ignoringElidableConstructorCall0"><pre>Matches expressions that match InnerMatcher that are possibly wrapped in an
-elidable constructor.
+elidable constructor and other corresponding bookkeeping nodes.
 
-In C++17 copy elidable constructors are no longer being
-generated in the AST as it is not permitted by the standard. They are
-however part of the AST in C++14 and earlier. Therefore, to write a matcher
-that works in all language modes, the matcher has to skip elidable
-constructor AST nodes if they appear in the AST. This matcher can be used to
-skip those elidable constructors.
+In C++17, elidable copy constructors are no longer being generated in the
+AST as it is not permitted by the standard. They are, however, part of the
+AST in C++14 and earlier. So, a matcher must abstract over these differences
+to work in all language modes. This matcher skips elidable constructor-call
+AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+various implicit nodes inside the constructor calls, all of which will not
+appear in the C++17 AST.
 
 Given
 
 struct H {};
 H G();
 void f() {
   H D = G();
 }
 
-``varDecl(hasInitializer(any(
-      ignoringElidableConstructorCall(callExpr()),
-      exprWithCleanups(ignoringElidableConstructorCall(callExpr()))))``
-matches ``H D = G()``
+``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr())))``
+matches ``H D = G()`` in C++11 through C++17 (and beyond).
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt;</td><td class="name" onclick="toggle('ignoringImpCasts0')"><a name="ignoringImpCasts0Anchor">ignoringImpCasts</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="ignoringImpCasts0"><pre>Matches expressions that match InnerMatcher after any implicit casts
 are stripped off.
 
 Parentheses and explicit casts are not discarded.

cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h

 /// \endcode
 AST_MATCHER(FunctionDecl, hasTrailingReturn) {
   if (const auto *F = Node.getType()->getAs<FunctionProtoType>())
     return F->hasTrailingReturn();
   return false;
 }
 
 /// Matches expressions that match InnerMatcher that are possibly wrapped in an
-/// elidable constructor.
+/// elidable constructor and other corresponding bookkeeping nodes.
 ///
-/// In C++17 copy elidable constructors are no longer being
-/// generated in the AST as it is not permitted by the standard. They are
-/// however part of the AST in C++14 and earlier. Therefore, to write a matcher
-/// that works in all language modes, the matcher has to skip elidable
-/// constructor AST nodes if they appear in the AST. This matcher can be used to
-/// skip those elidable constructors.
+/// In C++17, elidable copy constructors are no longer being generated in the
+/// AST as it is not permitted by the standard. They are, however, part of the
+/// AST in C++14 and earlier. So, a matcher must abstract over these differences
+/// to work in all language modes. This matcher skips elidable constructor-call
+/// AST nodes, `ExprWithCleanups` nodes wrapping elidable constructor-calls and
+/// various implicit nodes inside the constructor calls, all of which will not
+/// appear in the C++17 AST.
 ///
 /// Given
 ///
 /// \code
 /// struct H {};
 /// H G();
 /// void f() {
 ///   H D = G();
 /// }
 /// \endcode
 ///
-/// ``varDecl(hasInitializer(any(
-///       ignoringElidableConstructorCall(callExpr()),
-///       exprWithCleanups(ignoringElidableConstructorCall(callExpr()))))``
-/// matches ``H D = G()``
+/// ``varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr())))``
+/// matches ``H D = G()`` in C++11 through C++17 (and beyond).
 AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
-  if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(&Node)) {
+  // E tracks the node that we are examining.
+  const Expr *E = &Node;
+  // If present, remove an outer `ExprWithCleanups` corresponding to the
+  // underlying `CXXConstructExpr`. This check won't cover all cases of added
+  // `ExprWithCleanups` corresponding to `CXXConstructExpr` nodes (because the
+  // EWC is placed on the outermost node of the expression, which this may not
+  // be), but, it still improves the coverage of this matcher.
+  if (const auto *CleanupsExpr = dyn_cast<ExprWithCleanups>(&Node))
+    E = CleanupsExpr->getSubExpr();
+  if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(E)) {
     if (CtorExpr->isElidable()) {
       if (const auto *MaterializeTemp =
               dyn_cast<MaterializeTemporaryExpr>(CtorExpr->getArg(0))) {
         return InnerMatcher.matches(*MaterializeTemp->GetTemporaryExpr(),
                                     Finder, Builder);
       }
     }
   }

cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 TEST(Matcher, IgnoresElidableDoesNotPreventMatches) {
   EXPECT_TRUE(matches("void f() {"
                       "  int D = 10;"
                       "}",
                       expr(ignoringElidableConstructorCall(integerLiteral())),
                       LanguageMode::Cxx11OrLater));
 }
 
+TEST(Matcher, IgnoresElidableInVarInit) {
+  auto matcher =
+      varDecl(hasInitializer(ignoringElidableConstructorCall(callExpr())));
+  EXPECT_TRUE(matches("struct H {};"
+                      "H G();"
+                      "void f(H D = G()) {"
+                      "  return;"
+                      "}",
+                      matcher, LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(matches("struct H {};"
+                      "H G();"
+                      "void f() {"
+                      "  H D = G();"
+                      "}",
+                      matcher, LanguageMode::Cxx11OrLater));
+}
+
 TEST(Matcher, BindTheSameNameInAlternatives) {
   StatementMatcher matcher = anyOf(
     binaryOperator(hasOperatorName("+"),
                    hasLHS(expr().bind("x")),
                    hasRHS(integerLiteral(equals(0)))),
     binaryOperator(hasOperatorName("+"),
                    hasLHS(integerLiteral(equals(0))),
                    hasRHS(expr().bind("x"))));