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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 marked 2 inline comments as done.
usaxena95 added inline comments.


================
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.
----------------
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.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:108
 
+  void handleMacroOccurence(const MainFileMacros &MacroRefsToIndex);
+
----------------
ilya-biryukov wrote:
> 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.
> 
Renamed to `handleMacros` to avoid confusion with the virtual method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71406