[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/index/SymbolCollector.cpp:350
+  const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  if (!MainFileEntry)
+    return;
----------------
Does this ever happen?
Maybe `assert` it's not null instead?


================
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.
----------------
Won't we get a dangling pointer inside `Refs` after leaving this function?
MainFileURI gets deallocated at the end, right?



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:108
 
+  void handleMacroOccurence(const MainFileMacros &MacroRefsToIndex);
+
----------------
NIT: could you name it in plural? `handleMacros` or `handleMacroOccurences`?

It's different from the other version in this regard, which adds only a single occurrence.
It's also a good practice to avoid adding overloads to virtual/overriden methods, might get confusing at times.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71406