[clang-tools-extra] [clangd] Don't show inlay hints for __builtin_dump_struct (PR #71366)
Younan Zhang via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 20 08:20:20 PST 2023
https://github.com/zyn0217 updated https://github.com/llvm/llvm-project/pull/71366
>From 808c141c34218dd542b00149216adc061567dd31 Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Mon, 6 Nov 2023 16:50:02 +0800
Subject: [PATCH 1/5] [clangd] Don't show inlay hints for PseudoObjectExprs in
C++
This closes https://github.com/clangd/clangd/issues/1813.
PseudoObjectExprs in C++ are currently not very interesting but probably
mess up inlay hints.
---
clang-tools-extra/clangd/InlayHints.cpp | 13 ++++++++++
.../clangd/unittests/InlayHintTests.cpp | 24 +++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 3da047f95421385..867c70e5dbc80c6 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -589,6 +589,19 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
return true;
}
+ bool dataTraverseStmtPre(Stmt *S) {
+ // Do not show inlay hints for PseudoObjectExprs. They're never
+ // genuine user codes in C++.
+ //
+ // For example, __builtin_dump_struct would expand to a PseudoObjectExpr
+ // that includes a couple of calls to a printf function. Printing parameter
+ // names for that anyway would end up with duplicate parameter names (which,
+ // however, got de-duplicated after visiting) for the printf function.
+ if (AST.getLangOpts().CPlusPlus && isa<PseudoObjectExpr>(S))
+ return false;
+ return true;
+ }
+
bool VisitCallExpr(CallExpr *E) {
if (!Cfg.InlayHints.Parameters)
return true;
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 20c1cdd985dbc01..2b6c27b17b537a9 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1724,6 +1724,30 @@ TEST(InlayHints, RestrictRange) {
ElementsAre(labelIs(": int"), labelIs(": char")));
}
+TEST(ParameterHints, PseudoObjectExpr) {
+ Annotations Code(R"cpp(
+ struct S {
+ __declspec(property(get=GetX, put=PutX)) int x[];
+ int GetX(int y, int z) { return 42 + y; }
+ void PutX(int y) { x = y; } // Not `x = y: y`
+ };
+
+ int printf(const char *Format, ...);
+
+ int main() {
+ S s;
+ __builtin_dump_struct(&s, printf); // Not `Format: __builtin_dump_struct()`
+ printf($Param[["Hello, %d"]], 42); // Normal calls are not affected.
+ return s.x[1][2]; // Not `x[y: 1][z: 2]`
+ }
+ )cpp");
+ auto TU = TestTU::withCode(Code.code());
+ TU.ExtraArgs.push_back("-fms-extensions");
+ auto AST = TU.build();
+ EXPECT_THAT(inlayHints(AST, std::nullopt),
+ ElementsAre(HintMatcher(ExpectedHint{"Format: ", "Param"}, Code)));
+}
+
TEST(ParameterHints, ArgPacksAndConstructors) {
assertParameterHints(
R"cpp(
>From f15e35f67e23e742536ca2fe7229f5b6cba8f6b8 Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Mon, 6 Nov 2023 17:32:50 +0800
Subject: [PATCH 2/5] Don't make the patch C++-specific
---
clang-tools-extra/clangd/InlayHints.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 867c70e5dbc80c6..1b54b570c1c5d4f 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -591,13 +591,13 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
bool dataTraverseStmtPre(Stmt *S) {
// Do not show inlay hints for PseudoObjectExprs. They're never
- // genuine user codes in C++.
+ // genuine user codes.
//
// For example, __builtin_dump_struct would expand to a PseudoObjectExpr
// that includes a couple of calls to a printf function. Printing parameter
// names for that anyway would end up with duplicate parameter names (which,
// however, got de-duplicated after visiting) for the printf function.
- if (AST.getLangOpts().CPlusPlus && isa<PseudoObjectExpr>(S))
+ if (isa<PseudoObjectExpr>(S))
return false;
return true;
}
>From 8813a4f11254f89feb72e435888d896911a5dc78 Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Tue, 7 Nov 2023 14:22:07 +0800
Subject: [PATCH 3/5] Only apply the restriction to __builtin_dump_struct
---
clang-tools-extra/clangd/InlayHints.cpp | 18 +++++++++---------
.../clangd/unittests/InlayHintTests.cpp | 9 ++++++---
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 1b54b570c1c5d4f..9897d1c1e6da209 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -590,15 +590,15 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
}
bool dataTraverseStmtPre(Stmt *S) {
- // Do not show inlay hints for PseudoObjectExprs. They're never
- // genuine user codes.
- //
- // For example, __builtin_dump_struct would expand to a PseudoObjectExpr
- // that includes a couple of calls to a printf function. Printing parameter
- // names for that anyway would end up with duplicate parameter names (which,
- // however, got de-duplicated after visiting) for the printf function.
- if (isa<PseudoObjectExpr>(S))
- return false;
+ // Do not show inlay hints for the __builtin_dump_struct, which would expand
+ // to a PseudoObjectExpr that includes a couple of calls to a printf
+ // function. Printing parameter names for that anyway would end up with
+ // duplicate parameter names (which, however, got de-duplicated after
+ // visiting) for the printf function.
+ if (auto *POE = dyn_cast<PseudoObjectExpr>(S))
+ if (auto *CE = dyn_cast<CallExpr>(POE->getSyntacticForm());
+ CE && CE->getBuiltinCallee() == Builtin::BI__builtin_dump_struct)
+ return false;
return true;
}
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 2b6c27b17b537a9..47af261fad850e8 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1729,7 +1729,7 @@ TEST(ParameterHints, PseudoObjectExpr) {
struct S {
__declspec(property(get=GetX, put=PutX)) int x[];
int GetX(int y, int z) { return 42 + y; }
- void PutX(int y) { x = y; } // Not `x = y: y`
+ void PutX(int y) { x = $one[[y]]; } // FIXME: Undesired `x = y: y` for this ill-formed expression.
};
int printf(const char *Format, ...);
@@ -1738,14 +1738,17 @@ TEST(ParameterHints, PseudoObjectExpr) {
S s;
__builtin_dump_struct(&s, printf); // Not `Format: __builtin_dump_struct()`
printf($Param[["Hello, %d"]], 42); // Normal calls are not affected.
- return s.x[1][2]; // Not `x[y: 1][z: 2]`
+ return s.x[ $two[[1]] ][ $three[[2]] ]; // `x[y: 1][z: 2]`
}
)cpp");
auto TU = TestTU::withCode(Code.code());
TU.ExtraArgs.push_back("-fms-extensions");
auto AST = TU.build();
EXPECT_THAT(inlayHints(AST, std::nullopt),
- ElementsAre(HintMatcher(ExpectedHint{"Format: ", "Param"}, Code)));
+ ElementsAre(HintMatcher(ExpectedHint{"y: ", "one"}, Code),
+ HintMatcher(ExpectedHint{"Format: ", "Param"}, Code),
+ HintMatcher(ExpectedHint{"y: ", "two"}, Code),
+ HintMatcher(ExpectedHint{"z: ", "three"}, Code)));
}
TEST(ParameterHints, ArgPacksAndConstructors) {
>From c88aad6d290cb8e2732f7fae71cb2ea2ecf0abda Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Mon, 20 Nov 2023 00:14:17 +0800
Subject: [PATCH 4/5] Override TraversePseudoObjectExpr
---
clang-tools-extra/clangd/InlayHints.cpp | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 9897d1c1e6da209..b460baffee8802d 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -589,17 +589,22 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
return true;
}
- bool dataTraverseStmtPre(Stmt *S) {
- // Do not show inlay hints for the __builtin_dump_struct, which would expand
- // to a PseudoObjectExpr that includes a couple of calls to a printf
- // function. Printing parameter names for that anyway would end up with
- // duplicate parameter names (which, however, got de-duplicated after
+ bool TraversePseudoObjectExpr(PseudoObjectExpr *E) {
+ // Do not show inlay hints for the __builtin_dump_struct, which would
+ // expand to a PseudoObjectExpr that includes a couple of calls to a
+ // printf function. Printing parameter names for that anyway would end up
+ // with duplicate parameter names (which, however, got de-duplicated after
// visiting) for the printf function.
- if (auto *POE = dyn_cast<PseudoObjectExpr>(S))
- if (auto *CE = dyn_cast<CallExpr>(POE->getSyntacticForm());
- CE && CE->getBuiltinCallee() == Builtin::BI__builtin_dump_struct)
- return false;
- return true;
+ if (auto *CE = dyn_cast<CallExpr>(E->getSyntacticForm());
+ CE && CE->getBuiltinCallee() == Builtin::BI__builtin_dump_struct)
+ // Only traverse the syntactic forms. This leaves the door open in case
+ // the arguments in the syntactic form for __builtin_dump_struct could
+ // possibly get parameter names.
+ return RecursiveASTVisitor<InlayHintVisitor>::TraverseStmt(
+ E->getSyntacticForm());
+ // FIXME: Shall we ignore semantic forms for other pseudo object
+ // expressions?
+ return RecursiveASTVisitor<InlayHintVisitor>::TraversePseudoObjectExpr(E);
}
bool VisitCallExpr(CallExpr *E) {
>From ad5036adf86b4ea99cd3c3c231119ff48792ff6b Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Tue, 21 Nov 2023 00:19:12 +0800
Subject: [PATCH 5/5] Address some comments on the test.
---
clang-tools-extra/clangd/unittests/InlayHintTests.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 47af261fad850e8..f9f7fde36c14897 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1729,7 +1729,8 @@ TEST(ParameterHints, PseudoObjectExpr) {
struct S {
__declspec(property(get=GetX, put=PutX)) int x[];
int GetX(int y, int z) { return 42 + y; }
- void PutX(int y) { x = $one[[y]]; } // FIXME: Undesired `x = y: y` for this ill-formed expression.
+ // FIXME: Undesired hint `x = y: y`.
+ void PutX(int y) { x = $one[[y]]; }
};
int printf(const char *Format, ...);
@@ -1738,6 +1739,8 @@ TEST(ParameterHints, PseudoObjectExpr) {
S s;
__builtin_dump_struct(&s, printf); // Not `Format: __builtin_dump_struct()`
printf($Param[["Hello, %d"]], 42); // Normal calls are not affected.
+ // This builds a PseudoObjectExpr, but here it's useful for showing the
+ // arguments from the semantic form.
return s.x[ $two[[1]] ][ $three[[2]] ]; // `x[y: 1][z: 2]`
}
)cpp");
More information about the cfe-commits
mailing list