[PATCH] D116713: [clangd] Support configuration of inlay hints.
jackoalan added a comment. Assuming origin/maain is a branch typo and is no longer being tracked, would you please delete the upstream one in https://github.com/llvm/llvm-project/branches? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116713/new/ https://reviews.llvm.org/D116713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116713: [clangd] Support configuration of inlay hints.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG16fd5c278488: [clangd] Support configuration of inlay hints.
(authored by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -6,11 +6,13 @@
//
//===--===//
#include "Annotations.h"
+#include "Config.h"
#include "InlayHints.h"
#include "Protocol.h"
#include "TestTU.h"
#include "TestWorkspace.h"
#include "XRefs.h"
+#include "support/Context.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -24,6 +26,8 @@
namespace {
using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
std::vector Result;
@@ -56,6 +60,13 @@
MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
+Config noHintsConfig() {
+ Config C;
+ C.InlayHints.Parameters = false;
+ C.InlayHints.DeducedTypes = false;
+ return C;
+}
+
template
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
@@ -66,6 +77,10 @@
EXPECT_THAT(hintsOfKind(AST, Kind),
ElementsAre(HintMatcher(Expected, Source)...));
+ // Sneak in a cross-cutting check that hints are disabled by config.
+ // We'll hit an assertion failure if addInlayHint still gets called.
+ WithContextValue WithCfg(Config::Key, noHintsConfig());
+ EXPECT_THAT(inlayHints(AST, llvm::None), IsEmpty());
}
// Hack to allow expression-statements operating on parameter packs in C++14.
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -228,6 +228,23 @@
ASSERT_EQ(Results.size(), 1u);
EXPECT_THAT(Results[0].Hover.ShowAKA, llvm::ValueIs(Val(true)));
}
+
+TEST(ParseYAML, InlayHints) {
+ CapturedDiags Diags;
+ Annotations YAML(R"yaml(
+InlayHints:
+ Enabled: No
+ ParameterNames: Yes
+ )yaml");
+ auto Results =
+ Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ ASSERT_EQ(Results.size(), 1u);
+ EXPECT_THAT(Results[0].InlayHints.Enabled, llvm::ValueIs(Val(false)));
+ EXPECT_THAT(Results[0].InlayHints.ParameterNames, llvm::ValueIs(Val(true)));
+ EXPECT_EQ(Results[0].InlayHints.DeducedTypes, llvm::None);
+}
+
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -6,6 +6,7 @@
//
//===--===//
#include "InlayHints.h"
+#include "Config.h"
#include "HeuristicResolver.h"
#include "ParsedAST.h"
#include "clang/AST/DeclarationName.h"
@@ -23,8 +24,8 @@
class InlayHintVisitor : public RecursiveASTVisitor {
public:
InlayHintVisitor(std::vector , ParsedAST ,
- llvm::Optional RestrictRange)
- : Results(Results), AST(AST.getASTContext()),
+ const Config , llvm::Optional RestrictRange)
+ : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
RestrictRange(std::move(RestrictRange)),
MainFileID(AST.getSourceManager().getMainFileID()),
Resolver(AST.getHeuristicResolver()),
@@ -65,6 +66,9 @@
}
bool VisitCallExpr(CallExpr *E) {
+if (!Cfg.InlayHints.Parameters)
+ return true;
+
// Do not show parameter hints for operator calls written using operator
// syntax or user-defined literals. (Among other reasons, the resulting
// hints can look awkard, e.g. the expression can itself be a function
@@ -135,7 +139,7 @@
// the entire argument list likely appears in the main file and can be hinted.
void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
llvm::ArrayRef Args) {
-if (Args.size() == 0 || !Callee)
+if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
return;
// If the anchor location comes from a macro defintion, there's nowhere to
@@ -323,6
[PATCH] D116713: [clangd] Support configuration of inlay hints.
hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+ return true;
sammccall wrote:
> hokein wrote:
> > this should be `!Cfg.InlayHints.ParameterNames`.
> >
> > What do you think the idea of moving guards deeper (`processCall` and
> > `addTypeHint`)? The code seems clearer and we don't have to add them in all
> > Visit* implementation, this means that we pay cost of running some
> > necessary code, but I think it is trivial and probably worthy.
> I agree where to put the checks is an annoying question (and worth
> minimizing, since I've managed to get two wrong already).
> I do think there needs to be a pattern so we don't accidentally skip checks.
>
> - Easest is to postfilter (check in addInlayHint). That *does* hurt
> performance. Debug build on my laptop is ~170ms for all hints, ~160ms for
> postfilter with all categories disabled, ~5ms for current impl (prefilter)
> with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing
> traces)
> - Checking in VisitXXX methods (as done here) is a very regular pattern.
> Downside is you need many checks, and you can forget/break one
> - Checks in helpers so that one is always hit (as you propose) touches fewer
> places but it's ad-hoc. I'm afraid of getting it wrong as the impl is
> modified (missing checks, doing too much work first, etc).
> - Splitting RAV per category is an interesting option. Checking is very
> elegant, nicer code structure, can trace per-category latency, disabled
> categories can't crash... The downside is extra overhead, but this must be
> <5ms in the performance above. We can still choose to bundle simple
> categories together...
>
> I think I'll do as you propose for now, improve the tests, and refactor to
> try to make it feel less ad-hoc later.
> Also I should work out what we're spending 170ms on... EDIT: it turns out
> it's just assembling JSON objects :-\
Thanks for digging into this.
> ~5ms for current impl (prefilter) with all categories disabled. (On
> SemaCodeComplete.cpp, just eyeballing traces)
Yeah, we should have this prefilter implementation when all categories are
disabled.
> Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with
> all categories disabled,
The data doesn't seem reasonable, I think the total cost comes from two main
sources (AST traversal cost + assembling JSON object cost),
I would expect the `allhints` and `postfilter` have the similar AST traversal
cost, but `postfilter` should have a near zero cost of assembling JSON objectcs
(as it returns an empty vector), so the AST traversal cost is ~160ms.
> Also I should work out what we're spending 170ms on... EDIT: it turns out
> it's just assembling JSON objects :-\
And based on this, it turns out the cost of `all-hints` approach should be
larger than 170ms, which should be ~300ms (160ms for AST traversal + 170ms for
assembling JSON objects).
Anyway, the current implementation looks good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116713: [clangd] Support configuration of inlay hints.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+ return true;
hokein wrote:
> this should be `!Cfg.InlayHints.ParameterNames`.
>
> What do you think the idea of moving guards deeper (`processCall` and
> `addTypeHint`)? The code seems clearer and we don't have to add them in all
> Visit* implementation, this means that we pay cost of running some necessary
> code, but I think it is trivial and probably worthy.
I agree where to put the checks is an annoying question (and worth minimizing,
since I've managed to get two wrong already).
I do think there needs to be a pattern so we don't accidentally skip checks.
- Easest is to postfilter (check in addInlayHint). That *does* hurt
performance. Debug build on my laptop is ~170ms for all hints, ~160ms for
postfilter with all categories disabled, ~5ms for current impl (prefilter) with
all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)
- Checking in VisitXXX methods (as done here) is a very regular pattern.
Downside is you need many checks, and you can forget/break one
- Checks in helpers so that one is always hit (as you propose) touches fewer
places but it's ad-hoc. I'm afraid of getting it wrong as the impl is modified
(missing checks, doing too much work first, etc).
- Splitting RAV per category is an interesting option. Checking is very
elegant, nicer code structure, can trace per-category latency, disabled
categories can't crash... The downside is extra overhead, but this must be <5ms
in the performance above. We can still choose to bundle simple categories
together...
I think I'll do as you propose for now, improve the tests, and refactor to try
to make it feel less ad-hoc later.
Also I should work out what we're spending 170ms on... EDIT: it turns out it's
just assembling JSON objects :-\
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116713: [clangd] Support configuration of inlay hints.
sammccall updated this revision to Diff 398228.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Simplify & fix category checks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -6,11 +6,13 @@
//
//===--===//
#include "Annotations.h"
+#include "Config.h"
#include "InlayHints.h"
#include "Protocol.h"
#include "TestTU.h"
#include "TestWorkspace.h"
#include "XRefs.h"
+#include "support/Context.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -24,6 +26,8 @@
namespace {
using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
std::vector Result;
@@ -56,6 +60,13 @@
MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
+Config noHintsConfig() {
+ Config C;
+ C.InlayHints.Parameters = false;
+ C.InlayHints.DeducedTypes = false;
+ return C;
+}
+
template
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
@@ -66,6 +77,10 @@
EXPECT_THAT(hintsOfKind(AST, Kind),
ElementsAre(HintMatcher(Expected, Source)...));
+ // Sneak in a cross-cutting check that hints are disabled by config.
+ // We'll hit an assertion failure if addInlayHint still gets called.
+ WithContextValue WithCfg(Config::Key, noHintsConfig());
+ EXPECT_THAT(inlayHints(AST, llvm::None), IsEmpty());
}
// Hack to allow expression-statements operating on parameter packs in C++14.
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -228,6 +228,23 @@
ASSERT_EQ(Results.size(), 1u);
EXPECT_THAT(Results[0].Hover.ShowAKA, llvm::ValueIs(Val(true)));
}
+
+TEST(ParseYAML, InlayHints) {
+ CapturedDiags Diags;
+ Annotations YAML(R"yaml(
+InlayHints:
+ Enabled: No
+ ParameterNames: Yes
+ )yaml");
+ auto Results =
+ Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ ASSERT_EQ(Results.size(), 1u);
+ EXPECT_THAT(Results[0].InlayHints.Enabled, llvm::ValueIs(Val(false)));
+ EXPECT_THAT(Results[0].InlayHints.ParameterNames, llvm::ValueIs(Val(true)));
+ EXPECT_EQ(Results[0].InlayHints.DeducedTypes, llvm::None);
+}
+
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -6,6 +6,7 @@
//
//===--===//
#include "InlayHints.h"
+#include "Config.h"
#include "HeuristicResolver.h"
#include "ParsedAST.h"
#include "clang/AST/DeclarationName.h"
@@ -23,8 +24,8 @@
class InlayHintVisitor : public RecursiveASTVisitor {
public:
InlayHintVisitor(std::vector , ParsedAST ,
- llvm::Optional RestrictRange)
- : Results(Results), AST(AST.getASTContext()),
+ const Config , llvm::Optional RestrictRange)
+ : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
RestrictRange(std::move(RestrictRange)),
MainFileID(AST.getSourceManager().getMainFileID()),
Resolver(AST.getHeuristicResolver()),
@@ -65,6 +66,9 @@
}
bool VisitCallExpr(CallExpr *E) {
+if (!Cfg.InlayHints.Parameters)
+ return true;
+
// Do not show parameter hints for operator calls written using operator
// syntax or user-defined literals. (Among other reasons, the resulting
// hints can look awkard, e.g. the expression can itself be a function
@@ -135,7 +139,7 @@
// the entire argument list likely appears in the main file and can be hinted.
void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
llvm::ArrayRef Args) {
-if (Args.size() == 0 || !Callee)
+if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
return;
// If the anchor location comes from a macro defintion, there's nowhere to
@@ -323,6 +327,23 @@
void
[PATCH] D116713: [clangd] Support configuration of inlay hints.
hokein added inline comments.
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+ return true;
this should be `!Cfg.InlayHints.ParameterNames`.
What do you think the idea of moving guards deeper (`processCall` and
`addTypeHint`)? The code seems clearer and we don't have to add them in all
Visit* implementation, this means that we pay cost of running some necessary
code, but I think it is trivial and probably worthy.
Comment at: clang-tools-extra/clangd/InlayHints.cpp:69
bool VisitCallExpr(CallExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+ return true;
`!Cfg.InlayHints.ParameterNames`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116713: [clangd] Support configuration of inlay hints.
sammccall created this revision.
sammccall added reviewers: nridge, hokein.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
The idea is that the feature will always be advertised at the LSP level, but
depending on config we'll return partial or no responses.
We try to avoid doing the analysis for hints we're not going to return.
Examples of syntax:
# No hints
InlayHints:
Enabled: No
---
# Turn off a default category
InlayHints:
ParameterNames: No
---
# Turn on some categories
InlayHints:
ParameterNames: Yes
DeducedTypes: Yes
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116713
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -6,11 +6,13 @@
//
//===--===//
#include "Annotations.h"
+#include "Config.h"
#include "InlayHints.h"
#include "Protocol.h"
#include "TestTU.h"
#include "TestWorkspace.h"
#include "XRefs.h"
+#include "support/Context.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -23,6 +25,7 @@
namespace {
+using ::testing::IsEmpty;
using ::testing::UnorderedElementsAre;
std::vector hintsOfKind(ParsedAST , InlayHintKind Kind) {
@@ -49,6 +52,13 @@
arg.range == Code.range(Expected.RangeName);
}
+Config noHintsConfig() {
+ Config C;
+ C.InlayHints.Parameters = false;
+ C.InlayHints.DeducedTypes = false;
+ return C;
+}
+
template
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
@@ -59,6 +69,10 @@
EXPECT_THAT(hintsOfKind(AST, Kind),
UnorderedElementsAre(HintMatcher(Expected, Source)...));
+ // Sneak in a cross-cutting check that hints are disabled by config.
+ // We'll hit an assertion failure if addInlayHint still gets called.
+ WithContextValue WithCfg(Config::Key, noHintsConfig());
+ EXPECT_THAT(inlayHints(AST), IsEmpty());
}
template
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -228,6 +228,23 @@
ASSERT_EQ(Results.size(), 1u);
EXPECT_THAT(Results[0].Hover.ShowAKA, llvm::ValueIs(Val(true)));
}
+
+TEST(ParseYAML, InlayHints) {
+ CapturedDiags Diags;
+ Annotations YAML(R"yaml(
+InlayHints:
+ Enabled: No
+ ParameterNames: Yes
+ )yaml");
+ auto Results =
+ Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ ASSERT_EQ(Results.size(), 1u);
+ EXPECT_THAT(Results[0].InlayHints.Enabled, llvm::ValueIs(Val(false)));
+ EXPECT_THAT(Results[0].InlayHints.ParameterNames, llvm::ValueIs(Val(true)));
+ EXPECT_EQ(Results[0].InlayHints.DeducedTypes, llvm::None);
+}
+
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -6,6 +6,7 @@
//
//===--===//
#include "InlayHints.h"
+#include "Config.h"
#include "HeuristicResolver.h"
#include "ParsedAST.h"
#include "support/Logger.h"
@@ -20,8 +21,9 @@
class InlayHintVisitor : public RecursiveASTVisitor {
public:
- InlayHintVisitor(std::vector , ParsedAST )
- : Results(Results), AST(AST.getASTContext()),
+ InlayHintVisitor(std::vector , ParsedAST ,
+ const Config )
+ : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
MainFileID(AST.getSourceManager().getMainFileID()),
Resolver(AST.getHeuristicResolver()),
TypeHintPolicy(this->AST.getPrintingPolicy()),
@@ -46,6 +48,9 @@
}
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+if (!Cfg.InlayHints.DeducedTypes)
+ return true;
+
// Weed out constructor calls that don't look like a function call with
// an argument list, by checking the validity of getParenOrBraceRange().
// Also weed out std::initializer_list constructors as there are no names
@@ -61,6 +66,9 @@
}
bool VisitCallExpr(CallExpr *E) {
+if
