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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
