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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:55-59
+/// Matcher for gtest's `ON_CALL` macro. When `Args` is `NoMatchers`,
+/// this matches a mock call to a method without argument matchers e.g.
+/// `ON_CALL(mock, TwoParamMethod)`; when `Args` is `HasMatchers`, this
+/// matches a mock call to a method with argument matchers e.g.
+/// `ON_CALL(mock, TwoParamMethod(m1, m2))`. `MockObject` matches the mock
----------------
Please move this to a comment describing the `MockArgs` enum.


================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:34
 
+enum class MockArgs {
+  NoMatchers,
----------------
hokein wrote:
> 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. 
+1 Please move the comments relating to this enum from the two `gtestOnCall` functions to here.

hokein@ -- the "Matchers" is because its describing googletest matchers.  But, it is a bit confusing in this context and doesn't really add anything. What do you think of `None` and `Some` instead?


================
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) {
----------------
hokein wrote:
> the `static` qualifier is not needed as you wrap it within an anonymous namespace. the same below.
nit: per the style guide (https://releases.llvm.org/2.7/docs/CodingStandards.html#micro_anonns), I think it would be better to shrink the anonymous namespace to only enclose the enum decl, and keep these `static` annotations and in fact add a few that are currently missing on the `gtestCallInternal` functions.


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

https://reviews.llvm.org/D103195