[PATCH] D71406: [clangd] Add xref for macros to FileIndex.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:362
+      R.Location.End.setColumn(Range.end.character);
+      R.Location.FileURI = MainFileURI.c_str();
+      // FIXME: Add correct RefKind information to MainFileMacros.
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > Won't we get a dangling pointer inside `Refs` after leaving this function?
> > MainFileURI gets deallocated at the end, right?
> > 
> Yeah I also thought the same when I saw this at other places too. But the `Refs.insert(IDToRefs.first, R);` actually does a copy of this string and owns the copy.
Ah, makes sense. Thanks for explaining this.


================
Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:368
+  Index.updateMain(Test.Filename, AST);
+  // Add test2.cc
+  TestTU Test2;
----------------
Why do we need two tests? To test the file URI?
Setting a custom filename ("test.cc" in our case) should be enough for this.

I suggest removing `Test2` completely, it does not seem to test anything new and creates noise. Unless there are things I'm missing that it test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71406