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