[PATCH] D91626: [clangd] Implement textDocument/implementation.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

The patch description is a bit stale now.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1130
+  std::vector<LocatedSymbol> Results;
+  // We rely on index to find the implementations in subclasses.
+  if (!Index)
----------------
The current way is probably ok -- the index may be stale (if the index is not finished yet by the time we call find-implementation), so we might loose some latest results for the current main file. I think we need some comments describing the behavior. Maybe in future, we'll reconsider it


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1140
+  }
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+
----------------
nit: we need to verify `CurLoc` is available. 


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1160
+      auto DefLoc = indexToLSPLocation(Object.Definition, *MainFilePath);
+      Loc.Definition = DefLoc ? *DefLoc : *DeclLoc;
+      Results.push_back(Loc);
----------------
I think we can leave the Definition if `!DefLoc`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91626