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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
