[PATCH] D55818: [clangd] Unify path canonicalizations in the codebase

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The change mostly, the only important comment from me is about `toURI`. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.



================
Comment at: clangd/SourceCode.h:91
+/// possible.
+llvm::Expected<std::string> getCanonicalPath(const FileEntry *F,
+                                             const SourceManager &SourceMgr);
----------------
A different name proposal: `canonicalizePath`? WDYT?


================
Comment at: clangd/SourceCode.h:91
+/// possible.
+llvm::Expected<std::string> getCanonicalPath(const FileEntry *F,
+                                             const SourceManager &SourceMgr);
----------------
ilya-biryukov wrote:
> A different name proposal: `canonicalizePath`? WDYT?
Maybe leave it `Optional`?
Semantic differences make it very hard to verify the code after rewrite is correct, as `Expected` requires checking for errors.


================
Comment at: clangd/index/SymbolCollector.cpp:58
+  if (auto CanonPath =
+          getCanonicalPath(SM.getFileManager().getFile(Path), SM)) {
+    AbsolutePath = *CanonPath;
----------------
This looks like the only change that might subtly change the behavior of our program. I believe things might break because of this.
Could we keep this review closer to an NFC and avoid introducing this subtle difference here? Happy to take a look at a separate patch with it.


================
Comment at: clangd/index/SymbolCollector.cpp:62
-  if (sys::path::is_absolute(AbsolutePath)) {
-    // Handle the symbolic link path case where the current working directory
-    // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
----------------
This comment looks useful. Maybe keep it in the new implementation?


================
Comment at: clangd/index/SymbolCollector.cpp:64
   }
-
+  if (!sys::path::is_absolute(AbsolutePath) && !Opts.FallbackDir.empty())
+    sys::fs::make_absolute(Opts.FallbackDir, AbsolutePath);
----------------
Why not do this only in the `else` branch?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55818