New matcher for MaterializeTemporaryExpression.

[panzer]

Summary

A new matcher which corresponds to the MaterializeTemporaryExpression AST node.

[silvas]

It seems like it would almost make more sense to move these great doc comments with great examples of source constructs that the AST node corresponds to into the documentation for the AST nodes themselves, and then have single header (say, "StmtMatchers.h") with a block comment at the top saying something to the tune of

"each Stmt subclass has a corresponding matcher here, with a name which is the name of the AST node with the first letter lowercase rather than upper, so for example MaterializeTemporaryExpr's corresponding matcher is materializeTemporaryExpr. If you are unsure about what a given Stmt subclass corresponds to at the source level, each corresponding Stmt subclass's doxygen comment contains a number of examples which should help you get a feel for what the AST node corresponds to at the source level."

And then you could just stamp out all of them. I'm not sure how you're determining when to add exact AST node matchers, but I can't see a compelling reason to not just add all of them at once (well, besides the ever-present lack of time :). Of course, all of what I said for Stmt's could just as easily be said for Decl's or other AST nodes.

Maybe you could make it even more uniform from an API point of view and have a matcher astNode<T> where T is any AST node and which matches that node exactly, so the materializeTemporaryExpr you have in this patch could be written as astNode<MaterializeTemporaryExpr>. I think that from an API point of view this is really clean, since then the user only has to learn one matcher in order to match any exact AST node they want (this also seems like a huge win from a documentation perspective as well, since you now only need to document one matcher in order to cover all exact AST node matchers).

I understand that the astNode<T> is a bit more verbose, but the clarity and API transparency that it affords is I think much more important, since you can recover the syntax sugar trivially with aliases. For example, suppose that I just want to match records; clang has an AST node for that, which is RecordDecl, so I know I just have to do astNode<RecordDecl> and that's it. However, with the current design, I need to wade through docs to find it (it doesn't seem to exist after wading and grepping), or worse be deceived and use record(), which doesn't match RecordDecl's, but actually CXXRecordDecl's!

Is there something that I'm missing that complicates doing it that way? Could you compare/contrast your current approach with the one I just described?

In a sense, I guess what my radar is picking up on here is that you have some "boilerplate" AST matchers here which correspond exactly with AST nodes, while other ones don't, like allOf(), yet this distinction doesn't appear to be being called out in documentation or well-factored within the code itself (or documentation). Also, it seems like a documentation Single Point Of Truth violation to have these great examples of what the AST node corresponds to here and not on the actual AST node---these examples would probably also be of interest to a person reading documentation of the AST node. Having the examples here makes perfect sense for matchers which are not matching exact AST nodes, though.

Thanks for bearing with this novel.

[klimek]

include/clang/ASTMatchers/ASTMatchers.h

478

Please use \code around the code blocks.

unittests/ASTMatchers/ASTMatchersTest.cpp

1253

Is there a reason not to just inline this?

[panzer]

Added \code blocks and simplified test case matcher, as suggested.

[panzer]

Comments addressed in next revision.

include/clang/ASTMatchers/ASTMatchers.h

478

Done.

unittests/ASTMatchers/ASTMatchersTest.cpp

1253

Nope. Inlined.

[klimek]

lgtm apart from calling it materializeTemporaryExpr - when there is no abbr in the AST, I'm actually happy ;)

[klimek]

Landed as rL162609

include/clang/ASTMatchers/ASTMatchers.h

   Stmt,
   CXXBindTemporaryExpr> bindTemporaryExpression;
 
+/// \brief Matches nodes where temporaries are materialized.
+///
+/// Example: Given
+///   struct T {void func()};
+///   T f();
+///   void g(T);
+/// materializeTempExpr() matches 'f()' in these statements
+///   T u(f());
+///   g(f());
+/// but does not match
+///   f();
+///   f().func();
+const internal::VariadicDynCastAllOfMatcher<
+  Stmt,
+  MaterializeTemporaryExpr> materializeTempExpr;
+
 /// \brief Matches new expressions.
 ///
 /// Given

unittests/ASTMatchers/ASTMatchersTest.cpp

                  TempExpression));
 }
 
+TEST(MaterializeTempExpr, MatchesTemporary) {
+  StatementMatcher MaterializeTemp = materializeTempExpr();
+
+  std::string ClassString =
+      "class string { public: string(); int length(); }; ";
+
+  EXPECT_TRUE(
+      matches(ClassString +
+              "string GetStringByValue();"
+              "void FunctionTakesString(string s);"
+              "void run() { FunctionTakesString(GetStringByValue()); }",
+              MaterializeTemp));
+
+  EXPECT_TRUE(
+      notMatches(ClassString +
+                 "string* GetStringPointer(); "
+                 "void FunctionTakesStringPtr(string* s);"
+                 "void run() {"
+                 "  string* s = GetStringPointer();"
+                 "  FunctionTakesStringPtr(GetStringPointer());"
+                 "  FunctionTakesStringPtr(s);"
+                 "}",
+                 MaterializeTemp));
+
+  EXPECT_TRUE(
+      notMatches(ClassString +
+                 "string GetStringByValue();"
+                 "void run() { int k = GetStringByValue().length(); }",
+                 MaterializeTemp));
+
+  EXPECT_TRUE(
+      notMatches(ClassString +
+                 "string GetStringByValue();"
+                 "void run() { GetStringByValue(); }",
+                 MaterializeTemp));
+}
+
 TEST(ConstructorDeclaration, SimpleCase) {
   EXPECT_TRUE(matches("class Foo { Foo(int i); };",
                       constructor(ofClass(hasName("Foo")))));