[PATCH] D146244: [clangd] Show used symbols on #include line hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1158
+  const SourceManager &SM = AST.getSourceManager();
+  std::set<std::string> UsedSymbolNames;
+  include_cleaner::walkUsed(
----------------
VitaNuo wrote:
> hokein wrote:
> > just want to check the intention, we're using the `set` here because we want an alphabet order of `UsedSymbolNames`. If yes, then looks good (probably add a comment in the field `UsedSymbolNames` of HoverInfo).
> Actually no, we're using set primarily to deduplicate symbol names. Otherwise, many symbols have multiple references each, and the resulting list of used symbols ends up having a lot of duplicates.
> 
> I didn't know a set would order used symbols names alphabetically, but if so, that's a nice side effect :)
> 
> 
yeah, if duplication is the only purpose, then `llvm::DenseSet` (which is based on a hash-table, thus elements are not sorted) is preferred. The bonus point of using `std::set` (which is based on the red-black tree) is that the elements are sorted.

I think it would be nice to keep the result sorted (either by the ref range, or the symbol name), using symbol name seems fine to me, can you add a comment for UsedSymbolNames field to clarify that it is sorted by the symbol name?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1172
+        include_cleaner::Includes ConvertedIncludes =
+            convertIncludes(SM, llvm::ArrayRef{Inc});
+        for (const include_cleaner::Header &H : Providers) {
----------------
can you move it out of the callback? otherwise we will construct an `include_cleaner::Includes` every time when the callback is invoked, which is an unnecessary cost.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1176
+              ConvertedIncludes.match(H);
+          if (!Matches.empty()) {
+            UsedSymbolNames.insert(getRefName(Ref));
----------------
nit: inline the Matches in the `if` body, `if (!ConvertedIncludes.match(H).empty())`


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1444
+
+    if (UsedSymbolNames.size() >= SymbolNamesLimit) {
+      P.appendCode(UsedSymbolNames[SymbolNamesLimit - 1]);
----------------
I think it'd be better to move this into the above for loop (adding a special logic) rather than having a non-trivial handling for the last element.

What do you think about the something like below (it has less usages of `.size()` and `SymbolNamesLimit-1`)

```
auto Fronts = llvm::ArrayRef(UsedSymbolNames).take_front(std::min(UnusedSymbolNames.size(), SymbolNamesLimit));

for (const auto& Sym : Fronts) {
   P.appendCode(Sym);
   if (Sym != Fronts.back())
     P.appendText(", ");
}

if (UsedSymbolNames.size() > Fronts.size()) {
    P.appendText(" and ");
    P.appendText(std::to_string(UsedSymbolNames.size() - Fronts.size()));
    P.appendText(" more");
}
```


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:24
 #include <functional>
+#include <optional>
 #include <string>
----------------
the newly-introduced code doesn't seem to use the `std::optional` symbol


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2987
+    const std::function<void(HoverInfo &)> ExpectedBuilder;
+  } Cases[] = {{Annotations(R"cpp(
+                                        #inclu^de "foo.h"            
----------------
nit: fix the indentation (align with the one using in this file), see my comment in your other patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244