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