VitaNuo added a comment.
Thanks for the comments!
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1158
+ const SourceManager &SM = AST.getSourceManager();
+ std::set<std::string> UsedSymbolNames;
+ include_cleaner::walkUsed(
----------------
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 :)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1171
+ for (const include_cleaner::Header &H : Providers) {
+ switch (H.kind()) {
+ case include_cleaner::Header::Physical:
----------------
hokein wrote:
> 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.
Yeah, good idea, this makes the function considerably shorter.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1448
+
+ P.appendCode((*UsedSymbolNames)[UsedSymbolNames->size() - 1]);
+ if (UsedSymbolNames->size() > 5) {
----------------
hokein wrote:
> 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?
Right, sorry. That was only covering the case of UsedSymbolNames.size() <= 5.
================
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;
----------------
hokein wrote:
> 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.
Ok for `std::optional` removal, but I wouldn't want to use `llvm::SmallVector`.
`HoverInfo` does not contain any LLVM types atm (most of the fields are either
`std::string` or `std::vector`), so I am not sure we want to introduce a
divergence for unclear benefit.
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
