[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Haojian Wu via Phabricator via cfe-commits]

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