[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.
Herald added a subscriber: kadircet.


================
Comment at: clangd/SourceCode.cpp:209
+  llvm::SmallString<128> RealPath;
+  if (SourceMgr.getFileManager().getVirtualFileSystem()->getRealPath(
+          Path, RealPath)) {
----------------
With the recent performance regression due to `getRealPath()` mentioned in D51159, I think we should re-evaluate whether `VFS::getRealPath()`, which calls expensive `real_path` system call on real FS, was necessary to fix the bug mentioned in the patch summary. I think `vfs::makeAbsolutePath` with `remove_dot_dot` (without symlink resolution) should be sufficient to address the issue. The code completion latency has increased dramatically with the `real_path` call, so I would also expect `Xrefs`/`findDefinition` to slow down due to this.  As `SymbolCollector` is not in a latency sensitive code paths, it might be OK for it to call `real_path`, but I think we should try to avoid using `real_path` elsewhere.

In general, it's unclear whether clangd should always resolve symlink (it might not always be what users want), and it should probably be a decision made by the build system integration. I think we would need a more careful design if we decide to handle symlinks in clangd. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687