[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,
----------------
ymandel wrote:
> 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?
> What do you think of None and Some instead?

this sounds good to me.


================
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) {
----------------
ymandel wrote:
> 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.
Up to you -- I'm fine with either  (these two styles exit in LLVM codebase)- using anonymous namespace aligns more with google style...


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

https://reviews.llvm.org/D103195