hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1158
+ const SourceManager &SM = AST.getSourceManager();
+ std::set<std::string> UsedSymbolNames;
+ include_cleaner::walkUsed(
----------------
VitaNuo wrote:
> 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 :)
>
>
yeah, if duplication is the only purpose, then `llvm::DenseSet` (which is based
on a hash-table, thus elements are not sorted) is preferred. The bonus point of
using `std::set` (which is based on the red-black tree) is that the elements
are sorted.
I think it would be nice to keep the result sorted (either by the ref range, or
the symbol name), using symbol name seems fine to me, can you add a comment for
UsedSymbolNames field to clarify that it is sorted by the symbol name?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1172
+ include_cleaner::Includes ConvertedIncludes =
+ convertIncludes(SM, llvm::ArrayRef{Inc});
+ for (const include_cleaner::Header &H : Providers) {
----------------
can you move it out of the callback? otherwise we will construct an
`include_cleaner::Includes` every time when the callback is invoked, which is
an unnecessary cost.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1176
+ ConvertedIncludes.match(H);
+ if (!Matches.empty()) {
+ UsedSymbolNames.insert(getRefName(Ref));
----------------
nit: inline the Matches in the `if` body, `if
(!ConvertedIncludes.match(H).empty())`
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1444
+
+ if (UsedSymbolNames.size() >= SymbolNamesLimit) {
+ P.appendCode(UsedSymbolNames[SymbolNamesLimit - 1]);
----------------
I think it'd be better to move this into the above for loop (adding a special
logic) rather than having a non-trivial handling for the last element.
What do you think about the something like below (it has less usages of
`.size()` and `SymbolNamesLimit-1`)
```
auto Fronts =
llvm::ArrayRef(UsedSymbolNames).take_front(std::min(UnusedSymbolNames.size(),
SymbolNamesLimit));
for (const auto& Sym : Fronts) {
P.appendCode(Sym);
if (Sym != Fronts.back())
P.appendText(", ");
}
if (UsedSymbolNames.size() > Fronts.size()) {
P.appendText(" and ");
P.appendText(std::to_string(UsedSymbolNames.size() - Fronts.size()));
P.appendText(" more");
}
```
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:24
#include <functional>
+#include <optional>
#include <string>
----------------
the newly-introduced code doesn't seem to use the `std::optional` symbol
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2987
+ const std::function<void(HoverInfo &)> ExpectedBuilder;
+ } Cases[] = {{Annotations(R"cpp(
+ #inclu^de "foo.h"
----------------
nit: fix the indentation (align with the one using in this file), see my
comment in your other patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146244/new/
https://reviews.llvm.org/D146244
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
