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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/URI.h:115
+// the SourceManager.
+URI toURI(llvm::StringRef Path, const SourceManager &SM,
+          const std::string &FallbackDir);
----------------
This function does a very specialized form of path-to-uri conversion (fallback dirs, traversing schemes, etc.)
I'm afraid that naming it `toURI` will mean everyone will find and use it without giving it too much thought.

Another concern is layering. `URI.h` currently does not depend on `SourceManager` and, arguably, it should stay that way. We define an interface to extend the URI schemes that clangd supports, there's no reason for it to depend on `SourceManager`.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:86
+    // Add macro references.
+    for (const auto &IDToRefs : MacroRefsToIndex->MacroRefs) {
+      for (const auto &Range : IDToRefs.second) {
----------------
This is trying to emulate existing logic in `SymbolCollector::finish`. Is there a way we could share this?
Would avoid creating extra copies of reference slabs and allow to keep the code in one place, rather than scattering it between `FileIndex.cpp` and `SymbolCollector.cpp`. Would also allow to keep `toURI` private, meaning we don't have to worry about naming it and the fact it's exposed in the public interface.

One potential way to do this is to have an alternative version of `handleMacroOccurence`, which would fill `SymbolCollector::MacroRefs` directly and call this right after `indexTopLevelDecls`.
Would that work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71406