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

[Simon Marchi via Phabricator via cfe-commits]

simark marked 3 inline comments as done.
simark added a comment.

In https://reviews.llvm.org/D48687#1193470, @hokein wrote:

> Sorry for the delay. I thought there was a dependent patch of this patch, but it seems resolved, right?
>
> A rough round of review.


Right, the patch this one depends on is in clang, and there does not seem to be regressions this time.

>> New version. I tried to share the code between this and SymbolCollector, but
>>  didn't find a good clean way. Do you have concrete suggestions on how to do
>>  this? Otherwise, would it be acceptable to merge it as-is?
> 
> Yeah, I'm fine with the current change. I was not aware of the `getAbsoluteFilePath` has been pulled out to the `SourceCode.h`. I think the SymbolCollector could use this function as well (but that's out of scope of this patch).

Thanks.



================
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);
----------------
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.


================
Comment at: unittests/clangd/TestFS.cpp:52
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-                                  std::move(CommandLine), "")};
+  if (RelPathPrefix == StringRef()) {
+    // Use the absolute path in the compile command.
----------------
hokein wrote:
> Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`?
Done.


================
Comment at: unittests/clangd/XRefsTests.cpp:325
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
----------------
hokein wrote:
> It seems that there is no difference between `HeaderInPreambleAnnotations` and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated equally.
> 
The difference is that one is included in the main file as part of the preamble and the other is out of the preamble.  Since they take different code paths in clang, I think it's good to test both.  At some point, I had a similar bug that would only happen with a header included in the preamble.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687