nridge 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);
----------------
sammccall wrote:
> 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.
My instict is to keep the codepath exercised by the tests as close as possible
to the codepath exercised in production, because I've been bitten many times by
"1. Looks like we've regressed scenario X. 2. But we have a test for scenario
X. 3. Oh, but the test does something subtly different from what happens in
production." So, if in production we always pass a non-null
`HeuristicResolver`, I would prefer that we do so in the test as well.
Perhaps, as a compromise, we could annotate heuristic results as such (perhaps
using a new `DeclRelation::Heuristic` flag?), and check for the expected
presence / absence of that flag in the tests?
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
