zhaomo added inline comments.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:41
}
- llvm_unreachable("Unhandled GtestCmp enum");
}
----------------
hokein wrote:
> 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.
The switch statement there does not cover all the cases so `llvm_unreachable`
is necessary.
================
Comment at: clang/lib/ASTMatchers/GtestMatchers.cpp:104
+static internal::BindableMatcher<Stmt>
+gtestComparisonInternal(MacroType Macro, GtestCmp Cmp, StatementMatcher Left,
+ StatementMatcher Right) {
----------------
hokein wrote:
> 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.
Changed it a bit and moved it to the top of this file.
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
