Added AST matcher for ignoring elidable constructors

[jvikstrom]

Summary

Added AST matcher for ignoring elidable move constructors

[gribozavr]

clang/include/clang/ASTMatchers/ASTMatchers.h

6455

80 columns

6455

It would help if you described why a user would want to skip the elidable constructor using this matcher rather than hardcoding the presence of such an AST node.

6457

I'd suggest to rephrase the example using the "Given:" format (see other comments around), and wrap the code to 80 columns.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

575

Can you run these tests in both C++14 and C++17 modes?

628

Need a space before the open brace, and indentation in the function body (in every test).

631

Please clang-format.

[jvikstrom]

• Made ignoreElidable also ignore materializeTemporaryExpr and reformatted code

[gribozavr]

clang/include/clang/ASTMatchers/ASTMatchers.h

6455

"Matches expressions that match InnerMatcher that are possibly wrapped in an elidable constructor."

Then a blank line, and the rest of the explanation.

6458

part of the AST in C++14 and earlier.

6460

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.

6467

Can this sample be simplified? For example, B does not need to be a template (I think), and f() can be simplified to not call B twice.

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

628

Not done? I was talking about the code in the code snippet.

[hokein]

clang/include/clang/ASTMatchers/ASTMatchers.h

6477

Is the matcher only strict to move constructor? looks like the matcher implementation is for general constructors, just ignoringElidableConstructorCall? we may have elidable copy constructor.

6479

nit: LLVM uses CamelCase style.

6486

I think this return is unnecessary, since we will fall through at line 6489.

[jvikstrom]

• Updated example and fixed edge case in ignoringElidableConstructorCall

[jvikstrom]

• Using CamelCase and also renamed ignoringElidableMoveConstructorCall to ignoringElidableConstructorCall

[jvikstrom]

• Added match conditionally overload to control in what language standard a match should run in

[jvikstrom]

• Fixed wrong formatting in test code

[gribozavr]

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

600

Please inline the variables that are used once.

clang/unittests/ASTMatchers/ASTMatchersTest.h

61 ↗

(On Diff #204277)

Please drop the explicit values.

64 ↗

(On Diff #204277)

2a

68 ↗

(On Diff #204277)

"2a" (we don't know the year yet)

144 ↗

(On Diff #204277)

80 columns.

154 ↗

(On Diff #204277)

No need for semicolon.

157 ↗

(On Diff #204277)

I'd say it is too clever, and suggest to write an explicit switch.

[jvikstrom]

• Using switch for choosing language standard

[gribozavr]

clang/unittests/ASTMatchers/ASTMatchersTest.h

69 ↗

(On Diff #204282)

Cxx2aOrLater?

(no need to uppercase things)

159 ↗

(On Diff #204282)

The "orlater" variants should not appear here, please use "default: llvm_unreachable()".

[jvikstrom]

• Added default to switch for language modes

[jvikstrom]

• Updated outdated docs

[jvikstrom]

• Removed dependency on unordered map

[hokein]

looks most good to me, a few nits.

clang/include/clang/ASTMatchers/ASTMatchers.h

6488

This return statement is not needed.

clang/unittests/ASTMatchers/ASTMatchersTest.h

16 ↗

(On Diff #204442)

looks like the include is not used?

153 ↗

(On Diff #204442)

nit: add a llvm_unreachable for default.

177 ↗

(On Diff #204442)

nit: no {} needed for a single-statement body.

[jvikstrom]

• Removed unnecessary return

[hokein]

thanks, looks good to me. I'll commit for you. @gribozavr do you want to take another look on the patch?

clang/unittests/ASTMatchers/ASTMatchersTest.h

153 ↗

(On Diff #204442)

this is not done?

[jvikstrom]

clang/unittests/ASTMatchers/ASTMatchersTest.h

153 ↗

(On Diff #204442)

But the switch covers all enum values so that would give warnings

clang/docs/LibASTMatchersReference.html

Example matches x (matcher = expr(hasType(cxxRecordDecl(hasName("X")))))
             and friend class X (matcher = friendDecl(hasType("X"))
  class X {};
  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('ignoringElidableMoveConstructorCall0')"><a name="ignoringElidableMoveConstructorCall0Anchor">ignoringElidableMoveConstructorCall</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="ignoringElidableMoveConstructorCall0"><pre>Matches expressions that match InnerMatcher that are possibly wrapped in an
+elidable constructor.
+
+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.
+
+Given
+
+struct H {};
+H G();
+void f(){
+H D = G();
+}
+
+``varDecl(hasInitializer(ignoringElidableMoveConstructorCall(callExpr())))``
+matches ``H D = G()``
+</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.
 Given
   int arr[5];
   int a = 0;

clang/include/clang/ASTMatchers/ASTMatchers.h

 /// auto Y() -> int {}
 /// \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.
+///
+/// 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.
+///
+/// Given
+///
+/// \code
+/// struct H {};
+/// H G();
+/// void f(){
+/// H D = G();
+/// }
+/// \endcode
+///
+/// ``varDecl(hasInitializer(ignoringElidableMoveConstructorCall(callExpr())))``
+/// matches ``H D = G()``
+AST_MATCHER_P(Expr, ignoringElidableMoveConstructorCall,
+              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+
+  if (const auto *expr_cleanups = dyn_cast<ExprWithCleanups>(&Node)) {
+    bool matches = false;
+    for (const auto *obj : expr_cleanups->children()) {
+      if (const auto *cxx_construct_expr = dyn_cast<CXXConstructExpr>(obj)) {
+        matches = matches | ignoringElidableMoveConstructorCall(InnerMatcher)
+                                .matches(*cxx_construct_expr, Finder, Builder);
+      }
+    }
+    return matches;
+  }
+
+  if (const auto *cxx_construct_expr = dyn_cast<CXXConstructExpr>(&Node)) {
+    if (cxx_construct_expr->isElidable()) {
+      if (const auto *materialize_temp = dyn_cast<MaterializeTemporaryExpr>(
+              cxx_construct_expr->getArg(0))) {
+        const auto *first_child =
+            dyn_cast<Expr>(*materialize_temp->children().begin());
+        return InnerMatcher.matches(*first_child, Finder, Builder);
+      }
+      return InnerMatcher.matches(*cxx_construct_expr, Finder, Builder);
+    }
+  }
+  return InnerMatcher.matches(Node, Finder, Builder);
+}
+
 //----------------------------------------------------------------------------//
 // OpenMP handling.
 //----------------------------------------------------------------------------//
 
 /// Matches any ``#pragma omp`` executable directive.
 ///
 /// Given
 ///

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasTypeLoc);
   REGISTER_MATCHER(hasUnaryOperand);
   REGISTER_MATCHER(hasUnarySelector);
   REGISTER_MATCHER(hasUnderlyingDecl);
   REGISTER_MATCHER(hasUnderlyingType);
   REGISTER_MATCHER(hasUnqualifiedDesugaredType);
   REGISTER_MATCHER(hasValueType);
   REGISTER_MATCHER(ifStmt);
+  REGISTER_MATCHER(ignoringElidableMoveConstructorCall);
   REGISTER_MATCHER(ignoringImpCasts);
   REGISTER_MATCHER(ignoringImplicit);
   REGISTER_MATCHER(ignoringParenCasts);
   REGISTER_MATCHER(ignoringParenImpCasts);
   REGISTER_MATCHER(imaginaryLiteral);
   REGISTER_MATCHER(implicitCastExpr);
   REGISTER_MATCHER(implicitValueInitExpr);
   REGISTER_MATCHER(incompleteArrayType);

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

TEST(Matcher, BindMatchedNodes) {
   StatementMatcher MethodX =
     callExpr(callee(cxxMethodDecl(hasName("x")))).bind("x");
 
   EXPECT_TRUE(matchAndVerifyResultTrue("class A { void x() { x(); } };",
                                        MethodX,
                                        llvm::make_unique<VerifyIdIsBoundTo<CXXMemberCallExpr>>("x")));
 }
 
+TEST(Matcher, IgnoresElidableConstructors) {
+  auto matcher1 = cxxOperatorCallExpr(hasArgument(
+      1, callExpr(hasArgument(
+             0, ignoringElidableMoveConstructorCall(callExpr().bind("c"))))));
+  auto matcher2 = cxxOperatorCallExpr(hasArgument(
+      1, callExpr(hasArgument(0, ignoringElidableMoveConstructorCall(
+                                     integerLiteral().bind("c"))))));
+  auto matcher3 =
+      varDecl(hasInitializer(ignoringElidableMoveConstructorCall(callExpr())));
+
+  auto code1 = "struct H {};"
+               "template<typename T> H B(T A);"
+               "void f(){"
+               "H D1;"
+               "D1=B(B(1));"
+               "}";
+  auto code2 = "struct H {};"
+               "template<typename T> H B(T A);"
+               "void f(){"
+               "H D1;"
+               "D1=B(1);"
+               "}";
+  auto code3 = "struct H {};"
+               "H G();"
+               "void f(){"
+               "H D = G();"
+               "}";
+
+  EXPECT_TRUE(matchesConditionally(code1, matcher1, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code1, matcher1, true, "-std=c++17"));
+
+  EXPECT_TRUE(matchesConditionally(code2, matcher2, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code2, matcher2, true, "-std=c++17"));
+
+  EXPECT_TRUE(matchesConditionally(code3, matcher3, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code3, matcher3, true, "-std=c++17"));
+}
+
+TEST(Matcher, IgnoresElidableInReturn) {
+  auto matcher = expr(ignoringElidableMoveConstructorCall(declRefExpr()));
+  auto code1 = "struct H{};"
+               "H f(){"
+               "H g;"
+               "return g;"
+               "}";
+  auto code2 = "struct H{};"
+               "H f(){"
+               "return H();"
+               "}";
+  EXPECT_TRUE(matchesConditionally(code1, matcher, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code1, matcher, true, "-std=c++17"));
+  EXPECT_TRUE(matchesConditionally(code2, matcher, false, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code2, matcher, false, "-std=c++17"));
+}
+
+TEST(Matcher, IgnoreElidableConstructorDoesNotMatchConstructors) {
+  auto matcher = varDecl(
+      hasInitializer(ignoringElidableMoveConstructorCall(cxxConstructExpr())));
+  auto code = "struct H {};"
+              "void f(){"
+              "H D;"
+              "}";
+  EXPECT_TRUE(matchesConditionally(code, matcher, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code, matcher, true, "-std=c++17"));
+};
+
+TEST(Matcher, IgnoresElidableDoesNotPreventMatches) {
+  auto matcher = expr(ignoringElidableMoveConstructorCall(integerLiteral()));
+  auto code = "void f(){"
+              "int D = 10;"
+              "}";
+  EXPECT_TRUE(matchesConditionally(code, matcher, true, "-std=c++14"));
+  EXPECT_TRUE(matchesConditionally(code, matcher, true, "-std=c++17"));
+}
+
 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"))));