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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/SourceCode.cpp:203
+                FilePath)) {
+      elog("Could not turn relative path to absolute: {0}: {1}", FilePath,
+           EC.message());
----------------
Using `:` twice is a bit hard to read.
Maybe use something like `failed to turn relative path '{0}' to absolute: {1}` instead?


================
Comment at: clangd/SourceCode.cpp:205
+           EC.message());
+      return llvm::None;
     }
----------------
NIT: remove `llvm::` prefix in a cpp file. Same for other occurences.


================
Comment at: clangd/SourceCode.h:91
+/// possible.
+llvm::Expected<std::string> getCanonicalPath(const FileEntry *F,
+                                             const SourceManager &SourceMgr);
----------------
kadircet wrote:
> 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 ?
SG, not a big deal anyway. I have a personal bias against methods starting with `get` if they're not getter xD)


================
Comment at: clangd/index/SymbolCollector.cpp:58
+  if (auto CanonPath =
+          getCanonicalPath(SM.getFileManager().getFile(Path), SM)) {
+    AbsolutePath = *CanonPath;
----------------
kadircet wrote:
> 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.
Thanks for pointing me to the code I've missed


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55818