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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks for taking a look at this, this looks great!

mostly nits, but the one in tests is important and annoying (it might require you to update some existing cases)



================
Comment at: clang-tools-extra/clangd/Hover.cpp:680
 
+StringRef getAccessString(AccessSpecifier AS) {
+  switch (AS) {
----------------
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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:793
+  if (AccessSpecifier != AccessSpecifier::AS_none)
+    Header.appendText(getAccessString(AccessSpecifier)).appendSpace();
   if (Kind != index::SymbolKind::Unknown)
----------------
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.


================
Comment at: clang-tools-extra/clangd/Hover.h:63
+  /// Access specifier. Applies to members of class/structs or unions.
+  AccessSpecifier AccessSpecifier = AccessSpecifier::AS_none;
   /// Pretty-printed variable type.
----------------
Let's have `std::string` here, with a comment saying that `Access specifier for declarations inside class/struct/unions, empty for others.`


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:718
     EXPECT_EQ(H->Size, Expected.Size);
     EXPECT_EQ(H->Offset, Expected.Offset);
   }
----------------
could you also add an EXPECT_EQ for `H->AccessSpecifier` ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80472/new/

https://reviews.llvm.org/D80472