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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 18 12:17:13 PST 2021


sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:90
+    for (const auto &Entry :
+         allTargetDecls(N->ASTNode, AST.getHeuristicResolver()))
       ActualDecls.emplace_back(Entry.first, Entry.second);
----------------
nridge wrote:
> sammccall wrote:
> > If we really want these to be separate, I think we should be using a null resolver except for the tests that are explicitly testing heuristics resolution. (And those tests could check both with/without resolver and verify it's required...)
> I'm not sure what you mean by "if we really want these to be separate". What are "these"?
I mean the separation between the "hard" AST logic in FindTarget and the heuristic resolution. And by "we" I really mean "I", I guess :-)

Ideally we'd test these both separately and smoke-test the interaction, but I think the low-hanging fruit is to use all the existing tests, with most of them testing FindTarget in isolation and the few exceptions testing FindTarget+HeuristicResolver together.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92290/new/

https://reviews.llvm.org/D92290



More information about the cfe-commits mailing list