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

Viktoriia Bakalova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 13 09:38:33 PDT 2023


VitaNuo added a comment.

Thank you for the comments! I've implemented the new version as per the design doc, as well as did my best to address the review comments.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1093
+std::vector<include_cleaner::SymbolReference>
+collectMacroReferences(ParsedAST &AST) {
+  const auto &SM = AST.getSourceManager();
----------------
hokein wrote:
> On a second thought, I think we might not really need it. We handle the macro vs non-macro cases separately, and we only care about the symbol reference under the cursor.
> so we can simply the implementation:
> 
> 1) For macro case, we already have the located macro and Tok in the call site (on line1231), we can just construct a single `include_cleaner::SymbolReference` from them, and pass a single-element `Macros` and empty `ASTRoots` to the `walkUsed` API.
> 
> 2) For non-macro case, we can just pass an empty macro references to `walkUsed` as we already know the symbol under the hover is not a macro. 
This sounds like a great idea. Doing this now, thank you.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1128
+        if (Ref.RT != include_cleaner::RefType::Explicit)
+          // Ignore non-explicit references (e.g. variable names).
+          return;
----------------
hokein wrote:
> I don't get it, why variable names are non-explicit references?
I made a mistake in the wording, sorry. I meant something like this:
```
 std::string Hello = ◊"Hello";
```
 and wanted to prevent that hover over the string literal `"Hello"` shows `provided by <string>`. 

In the current implementation, however, this is not the case even without the explicit check for reference type.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1140
+        // Check that the user is hovering over the current symbol reference.
+        if (CurLoc < Ref.RefLocation || CurLoc > RefEndLocation) {
+          return;
----------------
hokein wrote:
> We're using the location as a heuristic to join the symbol (Decl) from two difference sources (clangd's hover impl, and include-cleaner lib impl)
> 
> This heuristics works most of time, but it will fail in the case where the symbol under the hover is a `UsingDecl`
> 
> ```
> // absl_string_view.h
> namespace absl {
> using std::string_view;
> }
> 
> // main.cpp
> #includle "absl_string_view.h"
> absl::str^ing_view abc; // hover on string_view
> ```
> 
> - clangd's hover uses the underlying decl (std::string_view), see the `pickDeclToUse` function for details;
> - include-cleaner lib reports the using decl (asbl::string_view);
> 
> As a result, we will show the #include of the using decl for the underlying decl in the hover card.
> 
> To solve the issue, I think we need to check the equality of Decl from hover and Decl from `SymbolReference` (comparing both `getCanonicalDecl` should be enough).
> 
Interesting case. I've tried it out, you're right indeed. And it seems like there's no way to pick the standard library header as provider, since include-cleaner doesn't report it (the absl header is the only candidate).

I guess we'll skip providing the header info in this case then.




================
Comment at: clang-tools-extra/clangd/Hover.cpp:1153
+                SM.getFileEntryForID(SM.getMainFileID()) ==
+                    Provider.physical()) {
+              // Do not report main file as provider.
----------------
hokein wrote:
> We probably don't need to handle this main-file case specially, in practice, main-file is hardly included in the MainFileIncludes, so the following logic will filter the main-file as well.
I believe we do need it. Otherwise, if you declare/define a new type in the main file (let's say `main.cpp`) and later use this type, you'd get `provided by main.cpp` in the hover, which seems pointless IMO.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1163
+            case include_cleaner::Header::Physical:
+              if (Provider.physical()->tryGetRealPathName() == Inc.Resolved)
+                DirectlyIncludedProviders.push_back(WrittenRef.str());
----------------
hokein wrote:
> checking the spelled string can cause subtle behavior & bugs. A robust way is to use the `include_cleaner::Includes.match API` to determine a header is used or not (this is similar to what you did in https://reviews.llvm.org/D143496, we need to expose the transformation function `convertIncludes`)
Using the `convertIncludes` function now for to find the best included provider.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1172
+              if (Provider.verbatim() == Inc.Written)
+                DirectlyIncludedProviders.push_back(WrittenRef.str());
+            }
----------------
hokein wrote:
> nit: I'd suggest adding a `break` even though it is the last case. 
This is gone now.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1181
+        HI.ProviderInfo = std::optional<std::vector<std::string>>{
+            std::move(DirectlyIncludedProviders)};
+      });
----------------
hokein wrote:
> I wonder whether we need to deduplicate the `DirectlyIncludedProviders`, reading the code, it might be possible that we add duplicated headers, but in practice, we should only have a single Ref reported, so the deduplication is probably not needed. 
Since we have decided that we always report one header, no de-duplication necessary anymore :)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1268
         // Look for a close enclosing expression to show the value of.
-        if (!HI->Value)
+        if (HI && !HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
----------------
hokein wrote:
> any reason add a null check here, the HI should always be non-null becase we have set it on Line1263.
sorry, probably some debugging artefact.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1335
 
+  if (ProviderInfo) {
+    Output.addRuler();
----------------
hokein wrote:
> Is the code position intended? It looks like we're inserting the `Provided by XXX` text in the middle of the function return type and the function parameters, which seems weird to me. I think the function return type and parameters should group together.
> 
> Depending on the final UI, I think moving it before the `ReturnType` (right after the header) seems better than the current one.
No, was not intended. We seem to have agreed to put it in the header section, so this should be the case now.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1340
+    P.appendSpace();
+    for (unsigned I = 0; I < ProviderInfo->size() - 1; I++) {
+      P.appendCode((*ProviderInfo)[I]);
----------------
hokein wrote:
> just use `llvm::join(*ProviderInfo, ", ");`
No need to join anything anymore, since we're showing a single header :)


================
Comment at: clang-tools-extra/clangd/Hover.h:71
+  /// Headers providing the symbol (directly included in the main file).
+  std::optional<std::vector<std::string>> ProviderInfo;
   std::optional<Range> SymRange;
----------------
hokein wrote:
> I think we can get rid of the `std::optional` we can distinguish by checking the `ProviderInfo.empty()`.
> 
> Since we have 1 header for most cases, let use `llvm::SmallVector`.
> 
> We can store the `Inclusion*` as the container element, and convert to string during the `present()` call.
Ok, now since we have 1 header in all cases, there's definitely no need for `std::optional`. Let's just store a string and check that it's not empty before rendering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144976



More information about the cfe-commits mailing list