hokein added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:34
+enum class MockArgs {
+ NoMatchers,
----------------
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.
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:39
+
/// Matcher for gtest's ASSERT_... macros.
internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher
Left,
----------------
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).
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:40
/// Matcher for gtest's ASSERT_... macros.
internal::BindableMatcher<Stmt> gtestAssert(GtestCmp Cmp, StatementMatcher
Left,
StatementMatcher Right);
----------------
nit: I would reorder APIs in these file, grouping by assert, expect, oncall.
```
...gtestAssert();
...gtestAssertThat();
...gtestExpect();
...gtestExpectThat();
...gtestExpectCall();
```
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:81
+
+/// Like the second `gtestOnCall` overload but for `EXPECT_CALL`.
+internal::BindableMatcher<Stmt> gtestExpectCall(StatementMatcher MockCall,
----------------
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.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
}
- llvm_unreachable("Unhandled GtestCmp enum");
}
----------------
why remove this `llvm_unreachable`? I think this is a common practice in LLVM.
================
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) {
----------------
the `static` qualifier is not needed as you wrap it within an anonymous
namespace. the same below.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:86
+ case MacroType::On:
+ return "InternalDefaultActionSetAt";
+ case MacroType::Expect:
----------------
worth comments on these magic string come from -- gtest library?
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:104
+static internal::BindableMatcher<Stmt>
+gtestComparisonInternal(MacroType Macro, GtestCmp Cmp, StatementMatcher Left,
+ StatementMatcher Right) {
----------------
As the function creates an AST matcher to match the gtest method, 'd rename the
function name like `gtestComparisonIMatcher`, the same to following functions.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:135
+ onImplicitObjectArgument(ignoringImplicit(MockCall)));
+ case MockArgs::HasMatchers:
+ return cxxMemberCallExpr(
----------------
I believe this is something related to the gtest internal implementation, could
you add a comment to explain that?
================
Comment at: clang/unittests/ASTMatchers/GtestMatchersTest.cpp:330
+ callee(functionDecl(hasName("gmock_TwoArgsMethod"))))
+ .bind("mock_call"),
+ MockArgs::NoMatchers)));
----------------
nit: bind is not needed?
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
