[cfe-commits] [PATCH] C++11 matchers

[Daniel Jasper]

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:678
@@ -669,1 +677,3 @@
 
+/// \brief Matches C++11 range for statements.
+///
----------------
Gábor Horváth wrote:
> Manuel Klimek wrote:
> > I'd cut the C++11 part out of the docs... Thoughts?
> I wasn't sure, if range for is explicit enough, to not confuse anybody with regular for. But I agree, it is a bit strange there.
The official term is "range-based for statement". Please use that.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:805
@@ +804,3 @@
+  Stmt,
+  CXXNullPtrLiteralExpr> nullPtrLiteralExpr;
+
----------------
Manuel Klimek wrote:
> Daniel Jasper wrote:
> > I wonder whether this should be nullPtrLiteral; for consistency with the other matchers. That would then (at a later point entail renaming the AST node). Manuel, any thoughts?
> I'd rather keep as much as possible in common with the AST nodes, so I'd vote for keeping it as proposed here.
ok.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:815
@@ +814,3 @@
+  EXPECT_TRUE(matches("#include <initializer_list>\n"
+                      "void f() { for (auto &a : {1, 2, 3}); }",
+                      forRangeStmt()));
----------------
Gábor Horváth wrote:
> Daniel Jasper wrote:
> > Add a test to and change comments of both the forStmt() and the forRangeStmt() matchers so that it is clear whether one matches the other or not.
> You mean that, I should add something like notMatches("old for snippet",forRangeStmt() ) and notMatches("range for snippet",forStmt())?
yes. And also include those cases in the comment documenting the matchers.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:820
@@ +819,3 @@
+TEST(Matcher, UserDefinedLiteral) {
+  EXPECT_TRUE(matches("constexpr char operator \"\" _inc (const char i)"
+                      "{ return i + 1; } char c = 'a'_inc;",
----------------
Gábor Horváth wrote:
> Daniel Jasper wrote:
> > I know that we don't do much formatting, but (as I am not yet very familiar with the C++11 feature), I found this quite hard to read. Could you at least add a newline after "}"?
> Indeed, the formatting of the code is not the best, I just did not want to violate the 80 column rule, and also did not want to have several lines of code. 
It is a trade-off, but in this case, I vote for one more line.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:2110
@@ -2088,3 +2109,3 @@
 TEST(Member, MatchesMemberAllocationFunction) {
-  EXPECT_TRUE(matches("namespace std { typedef typeof(sizeof(int)) size_t; }"
+  EXPECT_TRUE(matches("namespace std { typedef decltype(sizeof(int)) size_t; }"
                       "class X { void *operator new(std::size_t); };",
----------------
Gábor Horváth wrote:
> Daniel Jasper wrote:
> > I'd rather you don't change the content of this test. Rather use matchesConditionally() directly here.
> In this test I did not add a new parameter to matchesConditionally(), but of course, if you think it is cleaner that way, I will alter it. 
I don't want the test to essentially test a different thing, just because you changed the default language for matches().


http://llvm-reviews.chandlerc.com/D46