[PATCH] D103195: Add matchers for gtest's ASSERT_THAT, EXPECT_THAT, ON_CALL and EXPECT_CALL

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:34
 
+enum class MockArgs {
+  NoMatchers,
----------------
worth comments.

out of curiosity, what do we call this `Matchers`? I'd be careful to introduce a different matcher concept in the ast_matcher library, it would easily cause confusion. 


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:39
+
 /// Matcher for gtest's ASSERT_... macros.
 internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher Left,
----------------
as we add a new method to handle `ASSERT_THAT`, this comment is not clear enough to me, `ASSERT_...` makes me think `ASSERT_THAT` is included as well.

I would suggest rephrase the comment (explicitly mentioning this is for comparison operations, IIUC), and even rename the method to `gtestAssertCmp` (we can defer it to a follow-up patch). 


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:40
 /// Matcher for gtest's ASSERT_... macros.
 internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher Left,
                                             StatementMatcher Right);
----------------
nit: I would reorder APIs in these file, grouping by assert, expect, oncall.

```
...gtestAssert();
...gtestAssertThat();

...gtestExpect();
...gtestExpectThat();
...gtestExpectCall();

```


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:81
+
+/// Like the second `gtestOnCall` overload but for `EXPECT_CALL`.
+internal::BindableMatcher<Stmt> gtestExpectCall(StatementMatcher MockCall,
----------------
this comment doesn't seem to express enough information, what's the difference from the above one? I think adding an example would be helpful.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
   }
-  llvm_unreachable("Unhandled GtestCmp enum");
 }
----------------
why remove this `llvm_unreachable`? I think this is a common practice in LLVM.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:47
 
-static llvm::StringRef getAssertMacro(GtestCmp Cmp) {
-  switch (Cmp) {
-    case GtestCmp::Eq:
-      return "ASSERT_EQ";
-    case GtestCmp::Ne:
-      return "ASSERT_NE";
-    case GtestCmp::Ge:
-      return "ASSERT_GE";
-    case GtestCmp::Gt:
-      return "ASSERT_GT";
-    case GtestCmp::Le:
-      return "ASSERT_LE";
-    case GtestCmp::Lt:
-      return "ASSERT_LT";
+static llvm::StringRef getMacroTypeName(MacroType Macro) {
+  switch (Macro) {
----------------
the `static` qualifier is not needed as you wrap it within an anonymous namespace. the same below.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:86
+  case MacroType::On:
+    return "InternalDefaultActionSetAt";
+  case MacroType::Expect:
----------------
worth comments on these magic string come from -- gtest library?


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:104
+static internal::BindableMatcher<Stmt>
+gtestComparisonInternal(MacroType Macro, GtestCmp Cmp, StatementMatcher Left,
+                        StatementMatcher Right) {
----------------
As the function creates an AST matcher to match the gtest method, 'd rename the function name like `gtestComparisonIMatcher`, the same to following functions.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:135
+        onImplicitObjectArgument(ignoringImplicit(MockCall)));
+  case MockArgs::HasMatchers:
+    return cxxMemberCallExpr(
----------------
I believe this is something related to the gtest internal implementation, could you add a comment to explain that?


================
Comment at: clang/unittests/ASTMatchers/GtestMatchersTest.cpp:330
+                      callee(functionDecl(hasName("gmock_TwoArgsMethod"))))
+                      .bind("mock_call"),
+                  MockArgs::NoMatchers)));
----------------
nit: bind is not needed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103195/new/

https://reviews.llvm.org/D103195