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

[Zhaomo Yang via Phabricator via cfe-commits]

zhaomo added inline comments.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
   }
-  llvm_unreachable("Unhandled GtestCmp enum");
 }
----------------
hokein wrote:
> 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.
The switch statement there does not cover all the cases so `llvm_unreachable` is necessary.


================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:104
+static internal::BindableMatcher<Stmt>
+gtestComparisonInternal(MacroType Macro, GtestCmp Cmp, StatementMatcher Left,
+                        StatementMatcher Right) {
----------------
hokein wrote:
> 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.
Changed it a bit and moved it to the top of this file.


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

https://reviews.llvm.org/D103195