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

Gábor Horváth reviews at llvm-reviews.chandlerc.com
Thu Sep 20 09:51:25 PDT 2012



================
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;",
----------------
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. 

================
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()));
----------------
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())?

================
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); };",
----------------
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. 

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:678
@@ -669,1 +677,3 @@
 
+/// \brief Matches C++11 range for statements.
+///
----------------
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.


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



More information about the cfe-commits mailing list