[PATCH] D80472: [clangd] Add access specifier information to hover contents

[Daniel Martín via Phabricator via cfe-commits]

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