aaron.ballman added inline comments.
================ Comment at: clang-tidy/utils/Matchers.h:16 +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/CFG.h" ---------------- tbourvon wrote: > aaron.ballman wrote: > > This will require linking in the clangAnalysis library as well; are we sure > > we want to take on this dependency here for all matchers? > Do you have a specific solution in mind? We could make the matcher local to > the check it is being used in (see D37014), but I think it could prove useful > for other checks... No solution in mind -- I was mostly wondering if @alexfh was okay picking up this dependency or not, because it will impact all the clang-tidy modules. ================ Comment at: clang-tidy/utils/Matchers.h:58-60 + // We get the first parent, making sure that we're not in a case statement + // not in a compound statement directly inside a switch, because this causes + // the buildCFG call to crash. ---------------- tbourvon wrote: > aaron.ballman wrote: > > Why does it cause the crash? Should that crash not be fixed instead of > > applying this workaround? > I'm not entirely sure if this is expected behavior or not. In terms of AST, > `switch` statements are a bit special in terms of how they are represented > (each case contains all the subsequent cases as its children IIRC). > There probably is a way to make the CFG work in these cases, but I honestly > don't have the time to look into that and attempt a fix. Couldn't this be > good enough for now, maybe with a FIXME? I'm uncomfortable simply working around known crashes rather than fixing them, even with FIXME comments, because that just means we accrue more technical debt with no plan in place to pay it down in the future. However, I'll let @alexfh make the final call on it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42624 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
