hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land.
Looks good with a few nits. Looks like you didn't update the patch correctly. You have marked comments done, but your code doesn't get changed accordingly. Please double check with it. I tried your patch and it did fix the duplicated issue I encountered before. Thanks for fixing it! ================ Comment at: clangd/SourceCode.h:65 /// Get the absolute file path of a given file entry. -llvm::Optional<std::string> getAbsoluteFilePath(const FileEntry *F, - const SourceManager &SourceMgr); +llvm::Optional<std::string> getRealPath(const FileEntry *F, + const SourceManager &SourceMgr); ---------------- simark wrote: > hokein wrote: > > Why rename this function? > > > > Is it guaranteed that `RealPath` is always an absolute path? > Before, it only made sure that the path was absolute, now it goes a step > further and makes it a "real path", resolving symlinks and removing `.` and > `..`. When we talk about a "real path", it refers to the unix realpath > syscall: > > http://man7.org/linux/man-pages/man3/realpath.3.html > > So yes, the result is guaranteed to be absolute too. That makes sense, thanks for the explanation and the useful link! ================ Comment at: clangd/SourceCode.h:65 /// Get the absolute file path of a given file entry. TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, ---------------- nit: this comment is not needed. ================ Comment at: clangd/SourceCode.h:69 +llvm::Optional<std::string> getRealPath(const FileEntry *F, + const SourceManager &SourceMgr); ---------------- simark wrote: > ilya-biryukov wrote: > > This function looks like a good default choice for normalizing paths before > > putting them into LSP structs, ClangdServer responses, etc. > > I suggest we add a small comment here with a guideline for everyone to > > attempt using it whenever possible. WDYT? > What about this: > > ``` > /// Get the real/canonical path of \p F. This means: > /// > /// - Absolute path > /// - Symlinks resolved > /// - No "." or ".." component > /// - No duplicate or trailing directory separator > /// > /// This function should be used when sending paths to clients, so that paths > /// are normalized as much as possible. > ``` SG. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
