[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:1141
 
+std::string getRefName(include_cleaner::SymbolReference Ref) {
+  std::string Name;
----------------
we have a similar function `symbolName` in include-cleaner's `FindHeaders.cpp`, but it is not exposed. I think it would be nice to share the implementation (probably add a `name()` method in the `include_cleaner::Symbol` structure). No action required, can you add a FIXME here?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1145
+  case include_cleaner::Symbol::Declaration:
+    Name = llvm::dyn_cast<NamedDecl>(&Ref.Target.declaration())
+               ->getDeclName()
----------------
we need to check whether the dyn_cast is a null


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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1166
+        if (Ref.RT != include_cleaner::RefType::Explicit ||
+        !SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) {
+          return;
----------------
the indentation seems wrong here.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1171
+        for (const include_cleaner::Header &H : Providers) {
+          switch (H.kind()) {
+          case include_cleaner::Header::Physical:
----------------
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.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1448
+
+    P.appendCode((*UsedSymbolNames)[UsedSymbolNames->size() - 1]);
+    if (UsedSymbolNames->size() > 5) {
----------------
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?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1449
+    P.appendCode((*UsedSymbolNames)[UsedSymbolNames->size() - 1]);
+    if (UsedSymbolNames->size() > 5) {
+      P.appendText(" and ");
----------------
let's abstract a const variable e.g. `SymbolNamesLimit` for this magic number 5, and use it in all related places


================
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;
 
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2990
+  } Cases[] = {{Annotations(R"cpp(
+      [[#inclu^de "foo.h"]]            
+      
----------------
the `[[]]` range doesn't seem to be used, remove?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3000
+               {Annotations(R"cpp(
+      #include "foo.h"  
+      [[#include ^"bar.h"]]
----------------
nit: please fix the code indentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146244