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