ilya-biryukov added a comment. In D124441#3474123 <https://reviews.llvm.org/D124441#3474123>, @sammccall wrote:
> This looks great! > > Only thing I'm uncomfortable with is the lack of test coverage outside clangd. > I think the most valuable thing would be to have some direct testing of how > RecursiveASTVisitor behaves. Just to be clear, I feel it is not `RecursiveASTVisitor` that's at fault here. It's rather design of the Index library that splits work into multiple visitors. Requires expression is produces a template parameter list that suddenly appears out of nowhere in the `BodyVisitor`, whereas the code to handle it is in the `DeclVisitor`. > Do you think you could add something to > `unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp` at least for your > change and if there are any other bits that seem critical & missing? I actually think neither way to do the traversal works in all cases, so I'm reluctant to add a test, which feels like picking which behavior is better. The change I have avoids code duplication in the index library and is probably more convenient for source tooling in general. However, any code that needs to examine all elements of the AST for whatever reason (e.g. dumps or serialization) would probably be better off walking the synthesized template parameter list. The idea is: it's not important how exactly we traverse the nodes there as long as all interested visitors get callbacks for nodes they're looking into. The proper refactoring here would be to add `Traverse` and `Visit` methods for `concept::Requirement` and one for `ExprRequirement::ReturnRequirement`. Overriding `VisitReturnRequirement` in `BodyVisitor` would be enough and we can still visit the template parameter list like before. I'd like to do this in a separate change, would that work for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124441/new/ https://reviews.llvm.org/D124441 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
