[PATCH] D144976: [clangd] Add provider info on symbol hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, this looks great, some more comments (most are nits)



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+  const SourceManager &SM = AST.getSourceManager();
+  llvm::SmallVector<include_cleaner::Header> Headers =
+      include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes());
----------------
maybe name it `SortedProviders`, it contains the main-file (calling Headers is a bit confusing).


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1111
+      // Main file ranked higher than any #include'd file
+      break;
+
----------------
if we perform an early `return` inside the loop, the code can be simpler.

```
// Pickup the highest provider that satisfied the symbol usage, if available.
// If the main-file is the chosen provider, we deliberately do not display it (as it may cause too much noise, e.g. for local variables).
for (const auto & H : Headers) {
   if (isMainFile(H)) {
      return;
   }
   if (!Matches.empty()) {
      HI.Provider = Matches[0]->quote();
      return;
   } 
}

HI.Provider = spellHeader(AST...);
```


================
Comment at: clang-tools-extra/clangd/Hover.h:71
   std::string Name;
+  /// Header providing the symbol (best match).
+  std::string Provider;
----------------
nit: mention this header is with `""<>`.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:71
 
+include_cleaner::Includes
+convertIncludes(const SourceManager &SM,
----------------
nit: can you add some document comment on these two functions?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2896
+  struct Foo {};                     
+  Foo F = [[Fo^o]]{};
+)cpp"),
----------------
looks like `[[]]` is not used in the test, we only use the `^` point. Remove them?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2897
+  Foo F = [[Fo^o]]{};
+)cpp"),
+                [](HoverInfo &HI) { HI.Provider = ""; }},
----------------
I think making the indentation like below is cleaer:

```
{Annotations(R"cpp(
               struct Foo {}; 
               int F = [[FO^O]]{};
             )cpp"),
 [](HoverInfo &HI) { HI.Provider = ""; }},
```


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2911
+    #define FOO 5
+    int F = [[^FOO]];
+  )cpp"),
----------------
The indentation of the code block doesn't seem to align with the above cases. 

Unfortunately, clang-format doesn't format the codeblock inside the `cpp()cpp` string literal, we need to do it manually.





================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2938
+  )cpp"),
+                [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}};
+
----------------
I think this should be `""`, the using declaration is defined in main-file, main-file should be the only one provider, and we don't display main-file provider.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:16
 #include "clang-include-cleaner/Types.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
----------------
we have been using forward declaration for the `SourceLocation`, we can add a forward declaration `class SourceManager` on Line 26, and get rid of the #include here.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:84
+/// likely provider for the symbol.
+llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
+                                           const SourceManager &SM,
----------------
since we move it from `AnalysisInternal.h`, can you remove the one in `AnalysisInternal.h`? I think we need to add `#include "Analysis.h"` to `AnalysisInternal.h`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144976/new/

https://reviews.llvm.org/D144976