[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 a comment.

a few more nits, the code looks good to me now. As discussed with @ymandel offline, we should be aware of that before moving forward to this direction -- this patch will likely have the gtest-versioning issue.



================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:134
+// A ON_CALL or EXPECT_CALL macro expands to different AST structures depending
+// on whether the mock method has arguments or not. For example,
+// `ON_CALL(mock, TwoParamMethod)` is expanded to
----------------
I think `For example, ...` words are implementation details, should move to the corresponding `switch case` below.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
   }
-  llvm_unreachable("Unhandled GtestCmp enum");
 }
----------------
zhaomo wrote:
> hokein wrote:
> > why remove this `llvm_unreachable`? I think this is a common practice in LLVM.
> ymandel@ suggested me removing it as the switch covers all the possible values of the enum.
either seems fine to me (this is just a coding style), I think the problem is that we have inconsistencies in the patch - e.g. on Line95, `default: llvm_unreachable`, we should stick with one style.


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


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

https://reviews.llvm.org/D103195