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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added a comment.

Thanks for the comments!



================
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) {
----------------
hokein wrote:
> 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.
Oh right, sorry.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1444
+
+    if (UsedSymbolNames.size() >= SymbolNamesLimit) {
+      P.appendCode(UsedSymbolNames[SymbolNamesLimit - 1]);
----------------
hokein wrote:
> 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");
> }
> ```
Sure, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244