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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 7 inline comments as done.
kadircet added inline comments.


================
Comment at: clangd/SourceCode.h:91
+/// possible.
+llvm::Expected<std::string> getCanonicalPath(const FileEntry *F,
+                                             const SourceManager &SourceMgr);
----------------
ilya-biryukov wrote:
> 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.
but it rather gets the canonical path for a FileEntry rather than canonicalizing a path ?


================
Comment at: clangd/index/SymbolCollector.cpp:58
+  if (auto CanonPath =
+          getCanonicalPath(SM.getFileManager().getFile(Path), SM)) {
+    AbsolutePath = *CanonPath;
----------------
ilya-biryukov wrote:
> 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.
As discussed offline this is not introducing a behavior change.


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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55818