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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 18 08:15:15 PST 2018

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.

  rCTE Clang Tools Extra



More information about the cfe-commits mailing list