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