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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet marked an inline comment as done.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks! LGTM with some minor comments.

let me know if you don't have commit access so that i can land this for you.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:680
 
+StringRef getAccessString(AccessSpecifier AS) {
+  switch (AS) {
----------------
danielmartin wrote:
> 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`.
there's also some in https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/DeclPrinter.cpp#L291
- https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/DeclPrinter.cpp#L997
- https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/DeclPrinter.cpp#L432
- https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/JSONNodeDumper.h#L163
- https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/TextNodeDumper.h#L186


================
Comment at: clang-tools-extra/clangd/Hover.cpp:471
 
-  HI.AccessSpecifier = D->getAccess();
+  HI.AccessSpecifier = std::string(getAccess(D->getAccess()));
   HI.NamespaceScope = getNamespaceScope(D);
----------------
nit: you can do `getAccess(D->getAccess()).str()` (same for other places as well)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:616
          HI.Type = "typename";
+         HI.AccessSpecifier = "public";
        }},
----------------
this is surprising, but looks like sema always instantiates template parameters with public access specifiers explicitly.

no action needed, just leaving out comments in case we want to act on it in the future.


================
Comment at: clang/include/clang/Basic/Specifiers.h:369
+
+  inline llvm::StringRef getAccess(AccessSpecifier AS) {
+    switch (AS) {
----------------
`getAccessSpelling` instead of `getAccess`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80472