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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
