sammccall added a comment. In D56444#1351174 <https://reviews.llvm.org/D56444#1351174>, @steveire wrote:
> In D56444#1351145 <https://reviews.llvm.org/D56444#1351145>, @sammccall wrote: > > > This manifests as assertion failures (or with assertions off, incorrect > > results) for some matchers, on some code - people have encountered several > > examples where this happens, it's hard to know where to target a more > > targeted fix. > > The invariant violated is "RAV traverses every AST node (when implicit > > traversal is enabled)" - is there a way to fix this issue sufficiently > > without addressing that? > > > Yes - don't use RAV to traverse parents when AST-matching. OK, this is certainly a much more invasive change, and isn't going to make the release. I understand that you don't think this crash is worth fixing for 8.0, while others disagree. I don't particularly have a dog in this fight, but having seen now three reports of this crash makes me nervous about ignoring it. In D56444#1351182 <https://reviews.llvm.org/D56444#1351182>, @steveire wrote: > In D56444#1351150 <https://reviews.llvm.org/D56444#1351150>, @sammccall wrote: > > > In D56444#1351130 <https://reviews.llvm.org/D56444#1351130>, @steveire > > wrote: > > > > > In D56444#1351125 <https://reviews.llvm.org/D56444#1351125>, > > > @aaron.ballman wrote: > > > > > > > if the location isn't somewhere in user code, then don't consider the > > > > node or its children for traversal. However, that may be insufficient > > > > and equally as mysterious behavior. > > > > > > > > > That is exactly what I've implemented. I skip invisible nodes in matching > > > and dumping: > > > http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn > > > > > > So what happens when someone asks about the parent of an invisible node? > > > > e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the > > `operator()` function of a lambda class. (This is basically the case in the > > bug that this patch fixes) > > > Assuming that whether to skip invisible nodes is part of the `Ctx`, the `X` > would simply not be in context, just like if the `X` were not in the > `TraversalScope` of the `Ctx`. Ah, so any skipped nodes would not have any parents at all. The implementation of the idea would just be removing the assert - instead of assuming non-visited nodes is a bug, we assume they're *meant* to be excluded. However, such nodes are very easy to reach, inside or outside of a matcher. e.g. `callExpr(callee(functionDecl()))` will match a lambda call, and "func" will be the `operator()`. Having such nodes then not work with parent/ancestor matchers seems surprising and not **obviously** the best design choice (vs e.g. visiting all nodes including implicit, or breaking symmetry between downward and upward traversal). I guess what I'm saying is your suggestion seems like a significant change to the design of (ancestor) matchers, and that the approach in this patch is the more conservative one - making e.g. the implicit `FunctionDecl`s visible to matchers is consistent with `clang -ast-dump` and the behavior of matchers on other parts of the AST, even if that's something you'd like to change on an LLVM-9 timeline. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56444/new/ https://reviews.llvm.org/D56444 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
