[clangd] Show struct fields and enum members in hovers #89557
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Nathan Ridge (HighCommander4) ChangesFixes clangd/clangd#959 Full diff: https://github.com/llvm/llvm-project/pull/89557.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 06b949bc4a2b55..d7c433876b08bc 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -69,6 +69,8 @@ namespace {
PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
Base.AnonymousTagLocations = false;
Base.TerseOutput = true;
+ // Show struct fields and enum members.
+ Base.PrintTagTypeContents = true;
Base.PolishForDeclaration = true;
Base.ConstantsAsWritten = true;
Base.SuppressTemplateArgsInCXXConstructors = true;
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 35db757b9c15b5..8c19f5fb3b0ad0 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1645,7 +1645,16 @@ TEST(Hover, All) {
{
R"cpp(// Struct
namespace ns1 {
- struct MyClass {};
+ struct MyClass {
+ // Public fields shown in hover
+ int field1;
+ int field2;
+
+ // Methods and private fields not shown
+ void method();
+ private:
+ bool private_field;
+ };
} // namespace ns1
int main() {
ns1::[[My^Class]]* Params;
@@ -1655,7 +1664,7 @@ TEST(Hover, All) {
HI.Name = "MyClass";
HI.Kind = index::SymbolKind::Struct;
HI.NamespaceScope = "ns1::";
- HI.Definition = "struct MyClass {}";
+ HI.Definition = "struct MyClass {\n int field1;\n int field2;\n}";
}},
{
R"cpp(// Class
@@ -1685,7 +1694,7 @@ TEST(Hover, All) {
HI.Name = "MyUnion";
HI.Kind = index::SymbolKind::Union;
HI.NamespaceScope = "ns1::";
- HI.Definition = "union MyUnion {}";
+ HI.Definition = "union MyUnion {\n int x;\n int y;\n}";
}},
{
R"cpp(// Function definition via pointer
@@ -2030,7 +2039,7 @@ TEST(Hover, All) {
HI.Name = "Hello";
HI.Kind = index::SymbolKind::Enum;
HI.NamespaceScope = "";
- HI.Definition = "enum Hello {}";
+ HI.Definition = "enum Hello { ONE, TWO, THREE }";
HI.Documentation = "Enum declaration";
}},
{
diff --git a/clang/include/clang/AST/PrettyPrinter.h b/clang/include/clang/AST/PrettyPrinter.h
index da276e26049b00..6b8f1ae9143df3 100644
--- a/clang/include/clang/AST/PrettyPrinter.h
+++ b/clang/include/clang/AST/PrettyPrinter.h
@@ -70,7 +70,7 @@ struct PrintingPolicy {
Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
UseVoidForZeroParams(!LO.CPlusPlus),
SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
- PolishForDeclaration(false), Half(LO.Half),
+ PrintTagTypeContents(false), PolishForDeclaration(false), Half(LO.Half),
MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
MSVCFormatting(false), ConstantsAsWritten(false),
SuppressImplicitBase(false), FullyQualifiedName(false),
@@ -252,6 +252,14 @@ struct PrintingPolicy {
LLVM_PREFERRED_TYPE(bool)
unsigned TerseOutput : 1;
+ /// Print the contents of tag (i.e. record and enum) types, even with
+ /// TerseOutput=true.
+ ///
+ /// For record types (structs/classes/unions), this only prints public
+ /// data members. For enum types, this prints enumerators.
+ /// Has no effect if TerseOutput=false (which prints all members).
+ unsigned PrintTagTypeContents : 1;
+
/// When true, do certain refinement needed for producing proper declaration
/// tag; such as, do not print attributes attached to the declaration.
///
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 43d221968ea3fb..3fe02fff687fe1 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/PrettyPrinter.h"
#include "clang/Basic/Module.h"
+#include "clang/Basic/Specifiers.h"
#include "llvm/Support/raw_ostream.h"
using namespace clang;
@@ -464,8 +465,10 @@ void DeclPrinter::PrintConstructorInitializers(CXXConstructorDecl *CDecl,
//----------------------------------------------------------------------------
void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
- if (Policy.TerseOutput)
- return;
+ if (Policy.TerseOutput) {
+ if (!Policy.PrintTagTypeContents || !isa<TagDecl>(DC))
+ return;
+ }
if (Indent)
Indentation += Policy.Indentation;
@@ -474,6 +477,17 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
for (DeclContext::decl_iterator D = DC->decls_begin(), DEnd = DC->decls_end();
D != DEnd; ++D) {
+ // Print enum members and public struct fields when
+ // PrintTagTypeContents=true. Only applicable when TerseOutput=true since
+ // otherwise all members are printed.
+ if (Policy.TerseOutput) {
+ assert(Policy.PrintTagTypeContents);
+ if (!(isa<EnumConstantDecl>(*D) ||
+ (isa<FieldDecl>(*D) &&
+ dyn_cast<FieldDecl>(*D)->getAccess() == AS_public)))
+ continue;
+ }
+
// Don't print ObjCIvarDecls, as they are printed when visiting the
// containing ObjCInterfaceDecl.
if (isa<ObjCIvarDecl>(*D))
@@ -1182,7 +1196,7 @@ void DeclPrinter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// Print the class definition
// FIXME: Doesn't print access specifiers, e.g., "public:"
- if (Policy.TerseOutput) {
+ if (Policy.TerseOutput && !Policy.PrintTagTypeContents) {
Out << " {}";
} else {
Out << " {\n";
|
|
A few things I would appreciate feedback on:
|
clang/lib/AST/DeclPrinter.cpp
Outdated
| // PrintTagTypeContents=true. Only applicable when TerseOutput=true since | ||
| // otherwise all members are printed. | ||
| if (Policy.TerseOutput) { | ||
| assert(Policy.PrintTagTypeContents); |
There was a problem hiding this comment.
This assertion is doing nothing because we have exited early at the function beginning...?
clang/lib/AST/DeclPrinter.cpp
Outdated
| assert(Policy.PrintTagTypeContents); | ||
| if (!(isa<EnumConstantDecl>(*D) || | ||
| (isa<FieldDecl>(*D) && | ||
| dyn_cast<FieldDecl>(*D)->getAccess() == AS_public))) |
There was a problem hiding this comment.
Do you have any tests on C codes? Looking around the codebase, I think AS_none might be used in some cases wrt non-C++ codes?
There was a problem hiding this comment.
The patch does seem to work for me on C files, but adding a test to exercise the code in C mode is definitely a good idea. Thanks!
|
So my 2cents;
I feel like a more focused approach could still be preferred. E.g. what about showing this kind of "summary" only when hovering over the declaration of the symbol, rather than references? (similar to the way we show size/alignment info). As I believe showing For enabling it more generally, I still have some hesitations, as I am not sure how useful that interaction is in general. As not all editors let you interact with hover cards, and just seeing a bunch of field names, without any extra info, sounds suboptimal to me. I'd still prefer going over those fields via
As you might've sensed from the previous response, i'd definitely prefer a config option. As most terminal-based editors don't really have "rich" hovercard support, and even if we set the default to "verbose", i'd like to have a way for such people to keep using hover cards without definition eating up the whole screen estate.
I feel like "print only public fields" is too specific for clangd use case, and probably won't generalize to other callers at all. But I definitely agree with all the concerns around duplicating code. Looks like we have some |
Did you mean the other way around (references and not declaration, since when you're at the declaration you're likely already looking at the fields in the code)? Otherwise I'm missing the motivation for not showing it on references, which I think is the main part of the use case (being able to see this information at a glance when you're in a different file). |
This basically raises the question of what the function of a hover is, and I don't think I'd like to limit its scope to just type annotation, with all other information available either only by triggering autocompletion (a bit of a workaround that may require extra typing, even if all you want to do is get an idea about how a struct is structured) or by jumping to the definition. ClangD's behavior is also a bit inconsistent. In a hover, it will show documentation comments around the declaration, but not any of the content of the declaration. That means that it may eat up a lot of screen real estate if the documentation is extensive, but in the absence of documentation, it won't show much at all. Given that most terminal editors have either a gesture to show a hover or a gesture to dismiss one, or both, I'm not sure that avoiding using space for their sake really helps anybody. Given those premises, I'd like to suggest aiming to figure out whether there is some useful data about structs / enums / (maybe even) classes / functions that ClangD can straightforwardly derive and (compactly) display, and if so, what that is. Allowing the behavior to be configured makes a lot of sense, perhaps both booleanly and scalarly, i.e. whether to display such data at all, and if so, the maximum amount that should be displayed, and in my view should address most concerns about its impact on particular editors. |
Thanks for the suggestion! Using |
An RFC might be overkill, but asking for review from a clang code owner for changes to an interface such as
Yep, good call! |
|
I'm planning to revise the patch to make the following changes:
|
HighCommander4
force-pushed
the
issue-959
branch
from
d98c95b
to
fcb2ac4
Compare
These changes have been made in the latest version of the patch. |
Fixes clangd/clangd#959