[PATCH] D68977: [clangd] Report declaration references in findExplicitReferences.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:528
+  /// AST.
+  AllRefs annotateReferencesInMainAST(llvm::StringRef Code) {
+    TestTU TU;
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Why do we need this? Can't we test everything we do by putting into `foo`?
> > 
> > The original idea was to avoid too much output in the tests by letting the setup code live outside the function that's being annotated.
> > Any reason why we have to do the full file here?
> > 
> > I know it's a weird limitation, but that's one that was already made and I'd keep it this way if it's possible.
> I tried it but failed.
> 
> We can't test the qualified definitions (see example below) if we put everything into function `foo` (as defining a namespace is not allowed inside a function)
> 
> ```
> namespace $0^ns {
> class $1^Foo {
>  void $2^method();
> };
> void $3^func();
> }
> void $4^ns::$5^Foo::$6^method() {}
> void $7^ns::$8^func() {}
> ```
Fair point, but this patch does not add support for references in qualifiers of declarations.
I'd keep this change away.

We can totally test all things added by this patch inside the function body.

Could you split the part that adds tests for qualifiers into a separate change?
That would both make the current change more focused and allow to discuss alternatives for the testing story independently of the functionality we're adding here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68977