[PATCH] D115232: [clangd] Indexing of standard library

Wed Mar 30 14:14:18 PDT 2022

sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:313
+    llvm::StringRef DirPath = llvm::sys::path::parent_path(HeaderPath);
+    if (!HS.getFileMgr().getVirtualFileSystem().getRealPath(DirPath, Path))
+      SearchPaths.emplace_back(Path);
----------------
sammccall wrote:
> kadircet wrote:
> > why do we resolve the symlinks ?
> Oops, because I read the documentation of getCanonicalPath() instead of the implementation, and they diverged in https://github.com/llvm/llvm-project/commit/dd67793c0c549638cd93cad1142d324b979ba682 :-D
> 
> Ultimately we're forming URIs to lexically compare with the ones emitted by the indexer, which uses getCanonicalPath(). Today getCanonicalPath() wants a FileEntry and we don't have one, but I think there's no fundamental reason for that, I can fix it.
> 
> (I'll do it as a separate patch, for now it's just calling getCanonicalPath with the wrong arguments)
Actually, nevermind, the code is correct and I'd just forgotten why :-) Added a comment to StdLibLocation.

getCanonicalPath() does actually resolve symlinks and so on: it asks the FileManager for the directory entry and grabs the its "canonical name" which is just FS->getRealPath behind a cache.
So the strings are going to match the indexer after all.

It'd be possible to modify getCanonicalPath() so we can call it here, but I don't think it helps. Calling it with (path, filemanager) is inconvenient for the (many) existing callsites, so it'd have to be a new overload just for this case. And the FileManager caching we'd gain doesn't matter here.
I can still do it if you like, though.

(Also, relevant to your interests, realpath is probably taking care of case mismatches too!)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115232