njames93 marked 2 inline comments as done. njames93 added inline comments.
================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3296 BoundNodesTreeBuilder Result(*Builder); - auto MatchIt = matchesFirstInPointerRange(InnerMatcher, Node.method_begin(), - Node.method_end(), Finder, &Result); + auto MatchIt = matchesFirstInPointerRangeIgnoreUnspelled( + InnerMatcher, Node.method_begin(), Node.method_end(), Finder, &Result); ---------------- aaron.ballman wrote: > It amuses me that this lack of `const auto *` was not similarly flagged as > the one below, but feel free to correct this one as well. The reason it isn't flagged is because MatchIt is of type `specific_decl_iterator`, which isn't a pointer. ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:817-819 +/// Finds the first node in a pointer range that matches the given +/// matcher. Ignores any nodes that aren't spelled in source if Finder is ignore +/// them. ---------------- aaron.ballman wrote: > Comment looks like it can be re-flowed to 80 cols. > > "if Finder is ignore them" -> "if Finder is ignoring them" > > This comment is a bit odd because I would expect the same behavior from > `matchesFirstInPointerRange` -- e.g., if the finder says "ignore unless > spelled in source", I would not expect to have to call a function that also > says to honor that, so this feels a bit fragile. I was sort of thinking that > an (optional?) parameter to `matchesFirstInPointerPair` would be slightly > better (and reduce code duplication), but that feels similarly fragile. > > Would the logic be wrong to always honor what the `Finder` says about > ignoring implicit nodes? The issue is not all nodes have a nice way to say they are implicit and should be ignored. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97246/new/ https://reviews.llvm.org/D97246 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
