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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
