[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9510b0940265: [clangd] Factor out the heuristic resolver code into its own class (authored by nridge). Repository: rG LLVM Github Monorepo

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge updated this revision to Diff 323906. nridge added a comment. address nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 Files: clang-tools-extra/clangd/ASTSignals.cpp

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks so much for pushing this through, it looks great. In D92290#2564656 , @nridge wrote: > I haven't refactored the tests to pass in a null

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added a comment. Ok, I've made `HeuristicResolver` nullable, in the sense that I've added null checks arounds its use in `TargetFinder`. I haven't refactored the tests to pass in a null resolver / annotate the ones that do or don't need one. Would you like that done in this patch, or a

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge updated this revision to Diff 323886. nridge marked 10 inline comments as done. nridge added a comment. Rebase and address remaining comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 Files:

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

sammccall added a comment. Sorry for the looong delay getting back to this :-\ I do think this should be nullable (and const-friendly) but otherwise this is good to land. Comment at: clang-tools-extra/clangd/HeuristicResolver.h:73 + std::vector +

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added a comment. (This patch should be ready for another round of review.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 ___ cfe-commits mailing list

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added a comment. Thanks for the thoughtful comments about the API surface and such! I think I've addressed or responded to most of them. There are a few outstanding comments about how the HeuristicResolver object is stored and passed around (const, pointer, etc.), which I'll come back

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added inline comments. Comment at: clang-tools-extra/clangd/HeuristicResolver.h:60 + // or more declarations that it likely references. + std::vector resolveExprToDecls(const Expr *E); + sammccall wrote: > This is pretty vaguely defined. From reading

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge updated this revision to Diff 318910. nridge marked 4 inline comments as done. nridge added a comment. Address (many of the) review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 Files:

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:90 +for (const auto : + allTargetDecls(N->ASTNode, AST.getHeuristicResolver())) ActualDecls.emplace_back(Entry.first, Entry.second); sammccall

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

sammccall added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:90 +for (const auto : + allTargetDecls(N->ASTNode, AST.getHeuristicResolver())) ActualDecls.emplace_back(Entry.first, Entry.second); nridge

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

nridge added a comment. Thanks for the comments! I'll have a more detailed look later, but just wanted to ask one clarifying question for now: Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:90 +for (const auto : + allTargetDecls(N->ASTNode,

[PATCH] D92290: [clangd] Factor out the heuristic resolver code into its own class

sammccall added a comment. Thanks a lot for doing this! I have some thoughts on how we can give it a nice API/more clearly separate it from FindTarget/improve internal structure. I'm not sure how we should sequence that/how much to do. One the one hand, simply moving this code is progress! On