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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1158
+  const SourceManager &SM = AST.getSourceManager();
+  std::set<std::string> UsedSymbolNames;
+  include_cleaner::walkUsed(
----------------
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 :)




================
Comment at: clang-tools-extra/clangd/Hover.cpp:1171
+        for (const include_cleaner::Header &H : Providers) {
+          switch (H.kind()) {
+          case include_cleaner::Header::Physical:
----------------
hokein wrote:
> since we expose `convertIncludes` in your previous patch, we can construct a `include_cleaner::Includes` struct from the parssing `Inc`, and use the `match()` method to match the header.
Yeah, good idea, this makes the function considerably shorter.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1448
+
+    P.appendCode((*UsedSymbolNames)[UsedSymbolNames->size() - 1]);
+    if (UsedSymbolNames->size() > 5) {
----------------
hokein wrote:
> if the UsedSymbolNames.size() > 5, we will show the last element rather than the fifth one, my reading of the code this is not intended, right?
Right, sorry. That was only covering the case of UsedSymbolNames.size() <= 5.


================
Comment at: clang-tools-extra/clangd/Hover.h:116
+  // from a #include'd file that are used in the main file.
+  std::optional<std::vector<std::string>> UsedSymbolNames;
 
----------------
hokein wrote:
> we can use just a vector, we can use `.empty()` to the unused-include case.
> 
> `llvm::SmallVector` is preferred, per https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h.
Ok for `std::optional` removal, but I wouldn't want to use `llvm::SmallVector`. `HoverInfo` does not contain any LLVM types atm (most of the fields are either `std::string` or `std::vector`), so I am not sure we want to introduce a divergence for unclear benefit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244