zhaomo added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:39
+
/// Matcher for gtest's ASSERT_... macros.
internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher
Left,
----------------
hokein wrote:
> as we add a new method to handle `ASSERT_THAT`, this comment is not clear
> enough to me, `ASSERT_...` makes me think `ASSERT_THAT` is included as well.
>
> I would suggest rephrase the comment (explicitly mentioning this is for
> comparison operations, IIUC), and even rename the method to `gtestAssertCmp`
> (we can defer it to a follow-up patch).
I changed the comment to make it more accurate.
ymandel@ and I talked about rename the APIs but we weren't sure if it is worth
it as it may break some code. I would like to at least defer that to another
patch.
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:81
+
+/// Like the second `gtestOnCall` overload but for `EXPECT_CALL`.
+internal::BindableMatcher<Stmt> gtestExpectCall(StatementMatcher MockCall,
----------------
hokein wrote:
> this comment doesn't seem to express enough information, what's the
> difference from the above one? I think adding an example would be helpful.
The difference between the two `gtestExpectCall` overloads is just like the
difference between the two `gtestOnCall` overloads.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
}
- llvm_unreachable("Unhandled GtestCmp enum");
}
----------------
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.
================
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:
> 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...
I changed the code following ymandel@'s suggestion.
================
Comment at: clang/unittests/ASTMatchers/GtestMatchersTest.cpp:330
+ callee(functionDecl(hasName("gmock_TwoArgsMethod"))))
+ .bind("mock_call"),
+ MockArgs::NoMatchers)));
----------------
hokein wrote:
> nit: bind is not needed?
Just wanted to illustrate the usage here.
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
