sammccall added a comment. Ah, I suspected something like this was afoot! The patch looks... exciting :-) And I definitely agree we should remove the hacks if we're going to have access to ASTContext anyway.
I think an explicit parameter here is probably a good idea, it's a bit sad to lose targetDecl's simple signature but seems justified at this point. But **I also think we should consider splitting out the heuristic resolution parts into a separate module**, maybe leading to a signature like: `targetDecl(DynTypedNode N, HeuristicResolver * = nullptr)` The HeuristicResolver object could provide primitives like finding the pointee type, template patterns etc. It would hold a reference to Sema/ASTContext. There are a few reasons for this: - isolating complexity: FindTargets is mostly fairly broad-but-simple (many cases, but mostly trivial AST traversal) while heuristic resolution is fairly narrow-but-complex. Having the complexity in the way hinders understanding for people who, say, want to improve basic handling of some ObjC nodes that we forgot. - testing: Being able to test heuristic resolution primitives directly rather than through targetDecl seems valuable. Particularly for operations that involve sema and template instantiation, I expect we'll find crashes/problems and direct tests will expose them more clearly. Conversely the fallback heuristics can get in the way of testing the basic functionality (we've dealt with this a bit in xrefs). - encapsulation: I don't love the idea of exposing the Sema god-object everywhere via ParsedAST. We've gotten a long way without this, it's a really big and complicated hammer, and my suspicion is most places we'd be tempted to use it are bad or at least risky. If we expose HeuristicResolver from ParsedAST instead of Sema directly, we keep the barrier high. - reuse: might we also want to use the heuristic resolution for code completion at some point, e.g. LHS of MemberExpr? (We could abstract it via some callbacks, but I also suspect it's not coupled to clangd and could just be lifted up to clang at some point) What do you think about this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
