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 ↗

(On Diff #204097)

80 columns

6455 ↗

(On Diff #204097)

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 ↗

(On Diff #204097)

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 ↗

(On Diff #204097)

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

628 ↗

(On Diff #204097)

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

631 ↗

(On Diff #204097)

Please clang-format.

[jvikstrom]

• Made ignoreElidable also ignore materializeTemporaryExpr and reformatted code

[gribozavr]

clang/include/clang/ASTMatchers/ASTMatchers.h

6455 ↗

(On Diff #204229)

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

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

6458 ↗

(On Diff #204229)

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

6460 ↗

(On Diff #204229)

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 ↗

(On Diff #204229)

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 ↗

(On Diff #204097)

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

[hokein]

clang/include/clang/ASTMatchers/ASTMatchers.h

6477 ↗

(On Diff #204229)

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 ↗

(On Diff #204229)

nit: LLVM uses CamelCase style.

6486 ↗

(On Diff #204229)

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 ↗

(On Diff #204277)

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 ↗

(On Diff #204442)

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

cfe/trunk/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('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.
+
+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(any(
+      ignoringElidableConstructorCall(callExpr()),
+      exprWithCleanups(ignoringElidableConstructorCall(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;

cfe/trunk/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(any(
+///       ignoringElidableConstructorCall(callExpr()),
+///       exprWithCleanups(ignoringElidableConstructorCall(callExpr()))))``
+/// matches ``H D = G()``
+AST_MATCHER_P(Expr, ignoringElidableConstructorCall,
+              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(&Node)) {
+    if (CtorExpr->isElidable()) {
+      if (const auto *MaterializeTemp =
+              dyn_cast<MaterializeTemporaryExpr>(CtorExpr->getArg(0))) {
+        return InnerMatcher.matches(*MaterializeTemp->GetTemporaryExpr(),
+                                    Finder, Builder);
+      }
+    }
+  }
+  return InnerMatcher.matches(Node, Finder, Builder);
+}
+
 //----------------------------------------------------------------------------//
 // OpenMP handling.
 //----------------------------------------------------------------------------//
 
 /// Matches any ``#pragma omp`` executable directive.
 ///
 /// Given
 ///

cfe/trunk/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(ignoringElidableConstructorCall);
   REGISTER_MATCHER(ignoringImpCasts);
   REGISTER_MATCHER(ignoringImplicit);
   REGISTER_MATCHER(ignoringParenCasts);
   REGISTER_MATCHER(ignoringParenImpCasts);
   REGISTER_MATCHER(imaginaryLiteral);
   REGISTER_MATCHER(implicitCastExpr);
   REGISTER_MATCHER(implicitValueInitExpr);
   REGISTER_MATCHER(incompleteArrayType);

cfe/trunk/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) {
+  EXPECT_TRUE(
+      matches("struct H {};"
+              "template<typename T> H B(T A);"
+              "void f() {"
+              "  H D1;"
+              "  D1 = B(B(1));"
+              "}",
+              cxxOperatorCallExpr(hasArgument(
+                  1, callExpr(hasArgument(
+                         0, ignoringElidableConstructorCall(callExpr()))))),
+              LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(
+      matches("struct H {};"
+              "template<typename T> H B(T A);"
+              "void f() {"
+              "  H D1;"
+              "  D1 = B(1);"
+              "}",
+              cxxOperatorCallExpr(hasArgument(
+                  1, callExpr(hasArgument(0, ignoringElidableConstructorCall(
+                                                 integerLiteral()))))),
+              LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(matches(
+      "struct H {};"
+      "H G();"
+      "void f() {"
+      "  H D = G();"
+      "}",
+      varDecl(hasInitializer(anyOf(
+          ignoringElidableConstructorCall(callExpr()),
+          exprWithCleanups(has(ignoringElidableConstructorCall(callExpr())))))),
+      LanguageMode::Cxx11OrLater));
+}
+
+TEST(Matcher, IgnoresElidableInReturn) {
+  auto matcher = expr(ignoringElidableConstructorCall(declRefExpr()));
+  EXPECT_TRUE(matches("struct H {};"
+                      "H f() {"
+                      "  H g;"
+                      "  return g;"
+                      "}",
+                      matcher, LanguageMode::Cxx11OrLater));
+  EXPECT_TRUE(notMatches("struct H {};"
+                         "H f() {"
+                         "  return H();"
+                         "}",
+                         matcher, LanguageMode::Cxx11OrLater));
+}
+
+TEST(Matcher, IgnoreElidableConstructorDoesNotMatchConstructors) {
+  EXPECT_TRUE(matches("struct H {};"
+                      "void f() {"
+                      "  H D;"
+                      "}",
+                      varDecl(hasInitializer(
+                          ignoringElidableConstructorCall(cxxConstructExpr()))),
+                      LanguageMode::Cxx11OrLater));
+}
+
+TEST(Matcher, IgnoresElidableDoesNotPreventMatches) {
+  EXPECT_TRUE(matches("void f() {"
+                      "  int D = 10;"
+                      "}",
+                      expr(ignoringElidableConstructorCall(integerLiteral())),
+                      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"))));
TEST(IsNoThrow, MatchesNoThrowFunctionDeclarations) {
   EXPECT_TRUE(matches("void f() noexcept;", functionProtoType(isNoThrow())));
 }
 
 TEST(isConstexpr, MatchesConstexprDeclarations) {
   EXPECT_TRUE(matches("constexpr int foo = 42;",
                       varDecl(hasName("foo"), isConstexpr())));
   EXPECT_TRUE(matches("constexpr int bar();",
                       functionDecl(hasName("bar"), isConstexpr())));
-  EXPECT_TRUE(matchesConditionally("void baz() { if constexpr(1 > 0) {} }",
-                                   ifStmt(isConstexpr()), true, "-std=c++17"));
-  EXPECT_TRUE(matchesConditionally("void baz() { if (1 > 0) {} }",
-                                   ifStmt(isConstexpr()), false, "-std=c++17"));
+  EXPECT_TRUE(matches("void baz() { if constexpr(1 > 0) {} }",
+                      ifStmt(isConstexpr()), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }", ifStmt(isConstexpr()),
+                         LanguageMode::Cxx17OrLater));
 }
 
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
     matches("template<typename T> struct C {}; C<int> c;",
             classTemplateSpecializationDecl(templateArgumentCountIs(1))));
   EXPECT_TRUE(
     notMatches("template<typename T> struct C {}; C<int> c;",

cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.h

if (FindResultReviewer)
       FindResultReviewer->onEndOfTranslationUnit();
   }
 
 private:
   bool *const Verified;
   const std::unique_ptr<BoundNodesCallback> FindResultReviewer;
 };
 
+enum class LanguageMode {
+  Cxx11,
+  Cxx14,
+  Cxx17,
+  Cxx2a,
+  Cxx11OrLater,
+  Cxx14OrLater,
+  Cxx17OrLater,
+  Cxx2aOrLater
+};
+
 template <typename T>
 testing::AssertionResult matchesConditionally(
     const std::string &Code, const T &AMatcher, bool ExpectMatch,
     llvm::ArrayRef<llvm::StringRef> CompileArgs,
     const FileContentMappings &VirtualMappedFiles = FileContentMappings(),
     const std::string &Filename = "input.cc") {
   bool Found = false, DynamicFound = false;
   MatchFinder Finder;
testing::AssertionResult matchesConditionally(
     const FileContentMappings &VirtualMappedFiles = FileContentMappings(),
     const std::string &Filename = "input.cc") {
   return matchesConditionally(Code, AMatcher, ExpectMatch,
                               llvm::makeArrayRef(CompileArg),
                               VirtualMappedFiles, Filename);
 }
 
 template <typename T>
-testing::AssertionResult matches(const std::string &Code, const T &AMatcher) {
-  return matchesConditionally(Code, AMatcher, true, "-std=c++11");
+testing::AssertionResult
+matchesConditionally(const std::string &Code, const T &AMatcher,
+                     bool ExpectMatch, const LanguageMode &Mode) {
+  std::vector<LanguageMode> LangModes;
+  switch (Mode) {
+  case LanguageMode::Cxx11:
+  case LanguageMode::Cxx14:
+  case LanguageMode::Cxx17:
+  case LanguageMode::Cxx2a:
+    LangModes = {Mode};
+    break;
+  case LanguageMode::Cxx11OrLater:
+    LangModes = {LanguageMode::Cxx11, LanguageMode::Cxx14, LanguageMode::Cxx17,
+                 LanguageMode::Cxx2a};
+    break;
+  case LanguageMode::Cxx14OrLater:
+    LangModes = {LanguageMode::Cxx14, LanguageMode::Cxx17, LanguageMode::Cxx2a};
+    break;
+  case LanguageMode::Cxx17OrLater:
+    LangModes = {LanguageMode::Cxx17, LanguageMode::Cxx2a};
+    break;
+  case LanguageMode::Cxx2aOrLater:
+    LangModes = {LanguageMode::Cxx2a};
+  }
+
+  for (auto Mode : LangModes) {
+    std::string LangModeArg;
+    switch (Mode) {
+    case LanguageMode::Cxx11:
+      LangModeArg = "-std=c++11";
+      break;
+    case LanguageMode::Cxx14:
+      LangModeArg = "-std=c++14";
+      break;
+    case LanguageMode::Cxx17:
+      LangModeArg = "-std=c++17";
+      break;
+    case LanguageMode::Cxx2a:
+      LangModeArg = "-std=c++2a";
+      break;
+    default:
+      llvm_unreachable("Invalid language mode");
+    }
+
+    auto Result =
+        matchesConditionally(Code, AMatcher, ExpectMatch, LangModeArg);
+    if (!Result)
+      return Result;
+  }
+
+  return testing::AssertionSuccess();
+}
+
+template <typename T>
+testing::AssertionResult
+matches(const std::string &Code, const T &AMatcher,
+        const LanguageMode &Mode = LanguageMode::Cxx11) {
+  return matchesConditionally(Code, AMatcher, true, Mode);
 }
 
 template <typename T>
-testing::AssertionResult notMatches(const std::string &Code,
-                                    const T &AMatcher) {
-  return matchesConditionally(Code, AMatcher, false, "-std=c++11");
+testing::AssertionResult
+notMatches(const std::string &Code, const T &AMatcher,
+           const LanguageMode &Mode = LanguageMode::Cxx11) {
+  return matchesConditionally(Code, AMatcher, false, Mode);
 }
 
 template <typename T>
 testing::AssertionResult matchesObjC(const std::string &Code, const T &AMatcher,
                                      bool ExpectMatch = true) {
   return matchesConditionally(Code, AMatcher, ExpectMatch,
                               {"-fobjc-nonfragile-abi", "-Wno-objc-root-class",
                                "-fblocks", "-Wno-incomplete-implementation"},