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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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

A rough round of review.

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



================
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);
----------------
Why rename this function?

 Is it guaranteed that `RealPath` is always an absolute path?


================
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.
----------------
Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`?


================
Comment at: unittests/clangd/XRefsTests.cpp:325
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
----------------
It seems that there is no difference between `HeaderInPreambleAnnotations` and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated equally.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687