danielmartin marked 6 inline comments as done.
danielmartin added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:680
+StringRef getAccessString(AccessSpecifier AS) {
+ switch (AS) {
----------------
kadircet wrote:
> it is annoying to have this function duplicated in each component (there are
> duplicates at least in text and json node dumpers too) :/
>
> Feel free to provide a common implementation in
> `clang/include/clang/Basic/Specifiers.h` and migrate all other usages to it,
> or just put a FIXME in here saying we should converge those.
>
> nit: prefer `llvm::StringRef` as return type
I've only found usages in clang-doc, I don't know if there's more.
I've changed them to use the new common logic in `Specifiers.h`.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:793
+ if (AccessSpecifier != AccessSpecifier::AS_none)
+ Header.appendText(getAccessString(AccessSpecifier)).appendSpace();
if (Kind != index::SymbolKind::Unknown)
----------------
kadircet wrote:
> I wonder if it would be more natural to put this at bottom, where we list the
> containing class/struct/union. e.g.
>
> ```
> struct X { int fo^o; }
> ```
>
> would result in
>
> ```
> field foo
> ---
>
> // In X
> public: int foo
> ```
>
> I find current one useful too, just listing options to see what you (and
> possibly others interested in) think.
I think it's a bit less visible, but one good thing your suggestion has is that
in the Emacs client we use that part of the hover content to show a one liner
with type information. So I followed your suggestion to also have access
specifier information in our one-liner.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80472/new/
https://reviews.llvm.org/D80472
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
