hokein added a comment. Thanks, left a few comments on the implementation.
================ Comment at: clang-tools-extra/clangd/Hover.cpp:1091 +// FIXME(bakalova): Remove after merging https://reviews.llvm.org/D143496 +std::vector<include_cleaner::SymbolReference> ---------------- I'm not sure how we can do it. https://reviews.llvm.org/D143496 doesn't expose `collectMacroReferences` function, so we still duplicate this hacky implementations in two places, which is probably fine at the moment. I think this will be fixed when we unify them with clangd's `CollectMainFileMacros`. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1093 +std::vector<include_cleaner::SymbolReference> +collectMacroReferences(ParsedAST &AST) { + const auto &SM = AST.getSourceManager(); ---------------- 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. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1123 + if (!Ref.RefLocation.isFileID() || + !SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) + // Ignore locations within macro expansions or not in the main file ---------------- no need to do the isWrittenInMainFile check in the callback, the walkUsed implementation already does it. ================ 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; ---------------- I don't get it, why variable names are non-explicit references? ================ 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; ---------------- 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). ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1153 + SM.getFileEntryForID(SM.getMainFileID()) == + Provider.physical()) { + // Do not report main file as provider. ---------------- 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. ================ 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()); ---------------- 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`) ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1172 + if (Provider.verbatim() == Inc.Written) + DirectlyIncludedProviders.push_back(WrittenRef.str()); + } ---------------- nit: I'd suggest adding a `break` even though it is the last case. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1181 + HI.ProviderInfo = std::optional<std::vector<std::string>>{ + std::move(DirectlyIncludedProviders)}; + }); ---------------- 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. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1232 HI = getHoverContents(*M, Tok, AST); + if (HI) + maybeAddSymbolProviders(AST, *HI, *CurLoc); ---------------- nit: no need to check `HI`, it is always non-null as it is set by the above line. ================ 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()); ---------------- any reason add a null check here, the HI should always be non-null becase we have set it on Line1263. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1335 + if (ProviderInfo) { + Output.addRuler(); ---------------- 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. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1340 + P.appendSpace(); + for (unsigned I = 0; I < ProviderInfo->size() - 1; I++) { + P.appendCode((*ProviderInfo)[I]); ---------------- just use `llvm::join(*ProviderInfo, ", ");` ================ 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; ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144976/new/ https://reviews.llvm.org/D144976 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
