hokein added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/GtestMatchers.h:34
+enum class MockArgs {
+ NoMatchers,
----------------
ymandel wrote:
> 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?
> What do you think of None and Some instead?
this sounds good to me.
================
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) {
----------------
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...
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
