[clangd] Add inlay hints for default function arguments #95712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Tor Shepherd (torshepherd) ChangesTitle. This PR adds support for showing implicit lambda captures:
and defaulted function arguments:
as inlay hints. If the list of captures (or default args) is too long (based on TypeNameLimit), shows "..." instead:
Note: I wrote this before #85497 landed. With this we should probably also follow up with go-to-definition on the lambda captures at least; I could see that being helpful Full diff: https://github.com/llvm/llvm-project/pull/95712.diff 8 Files Affected:
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index 41143b9ebc8d2..5100214244454 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -148,6 +148,8 @@ struct Config {
bool DeducedTypes = true;
bool Designators = true;
bool BlockEnd = false;
+ bool LambdaCaptures = false;
+ bool DefaultArguments = false;
// Limit the length of type names in inlay hints. (0 means no limit)
uint32_t TypeNameLimit = 32;
} InlayHints;
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index f32f674443ffe..dcdb71ea01bb2 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -43,7 +43,6 @@
#include "llvm/Support/Regex.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
-#include <algorithm>
#include <memory>
#include <optional>
#include <string>
@@ -504,10 +503,10 @@ struct FragmentCompiler {
auto Fast = isFastTidyCheck(Str);
if (!Fast.has_value()) {
diag(Warning,
- llvm::formatv(
- "Latency of clang-tidy check '{0}' is not known. "
- "It will only run if ClangTidy.FastCheckFilter is Loose or None",
- Str)
+ llvm::formatv("Latency of clang-tidy check '{0}' is not known. "
+ "It will only run if ClangTidy.FastCheckFilter is "
+ "Loose or None",
+ Str)
.str(),
Arg.Range);
} else if (!*Fast) {
@@ -654,6 +653,16 @@ struct FragmentCompiler {
Out.Apply.push_back([Value(**F.BlockEnd)](const Params &, Config &C) {
C.InlayHints.BlockEnd = Value;
});
+ if (F.LambdaCaptures)
+ Out.Apply.push_back(
+ [Value(**F.LambdaCaptures)](const Params &, Config &C) {
+ C.InlayHints.LambdaCaptures = Value;
+ });
+ if (F.DefaultArguments)
+ Out.Apply.push_back(
+ [Value(**F.DefaultArguments)](const Params &, Config &C) {
+ C.InlayHints.DefaultArguments = Value;
+ });
if (F.TypeNameLimit)
Out.Apply.push_back(
[Value(**F.TypeNameLimit)](const Params &, Config &C) {
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index f3e51a9b6dbc4..ad30b43f34874 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -331,6 +331,11 @@ struct Fragment {
std::optional<Located<bool>> Designators;
/// Show defined symbol names at the end of a definition block.
std::optional<Located<bool>> BlockEnd;
+ /// Show names of captured variables by default capture groups in lambdas.
+ std::optional<Located<bool>> LambdaCaptures;
+ /// Show parameter names and default values of default arguments after all
+ /// of the explicit arguments.
+ std::optional<Located<bool>> DefaultArguments;
/// Limit the length of type name hints. (0 means no limit)
std::optional<Located<uint32_t>> TypeNameLimit;
};
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 3e9b6a07d3b32..58e5f8bead101 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -14,7 +14,6 @@
#include "llvm/Support/YAMLParser.h"
#include <optional>
#include <string>
-#include <system_error>
namespace clang {
namespace clangd {
@@ -264,6 +263,14 @@ class Parser {
if (auto Value = boolValue(N, "BlockEnd"))
F.BlockEnd = *Value;
});
+ Dict.handle("LambdaCaptures", [&](Node &N) {
+ if (auto Value = boolValue(N, "LambdaCaptures"))
+ F.LambdaCaptures = *Value;
+ });
+ Dict.handle("DefaultArguments", [&](Node &N) {
+ if (auto Value = boolValue(N, "DefaultArguments"))
+ F.DefaultArguments = *Value;
+ });
Dict.handle("TypeNameLimit", [&](Node &N) {
if (auto Value = uint32Value(N, "TypeNameLimit"))
F.TypeNameLimit = *Value;
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index cd4f1931b3ce1..2631c17409eab 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -11,27 +11,37 @@
#include "Config.h"
#include "HeuristicResolver.h"
#include "ParsedAST.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "clang/AST/ASTDiagnostic.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/LambdaCapture.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/StmtVisitor.h"
#include "clang/AST/Type.h"
#include "clang/Basic/Builtins.h"
+#include "clang/Basic/Lambda.h"
#include "clang/Basic/OperatorKinds.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+#include <iterator>
#include <optional>
#include <string>
@@ -372,6 +382,34 @@ maybeDropCxxExplicitObjectParameters(ArrayRef<const ParmVarDecl *> Params) {
return Params;
}
+llvm::StringRef getLambdaCaptureName(const LambdaCapture &Capture) {
+ if (Capture.capturesVariable())
+ return Capture.getCapturedVar()->getName();
+ if (Capture.capturesThis())
+ return llvm::StringRef{"this"};
+ return llvm::StringRef{"unknown"};
+}
+
+template <typename R, typename P>
+std::string joinAndTruncate(R &&Range, size_t MaxLength,
+ P &&GetAsStringFunction) {
+ std::string Out;
+ bool IsFirst = true;
+ for (auto &&Element : Range) {
+ if (!IsFirst)
+ Out.append(", ");
+ else
+ IsFirst = false;
+ auto AsString = GetAsStringFunction(Element);
+ if (Out.size() + AsString.size() >= MaxLength) {
+ Out.append("...");
+ break;
+ }
+ Out.append(AsString);
+ }
+ return Out;
+}
+
struct Callee {
// Only one of Decl or Loc is set.
// Loc is for calls through function pointers.
@@ -422,7 +460,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
Callee.Decl = E->getConstructor();
if (!Callee.Decl)
return true;
- processCall(Callee, {E->getArgs(), E->getNumArgs()});
+ processCall(Callee, E->getParenOrBraceRange().getEnd(),
+ {E->getArgs(), E->getNumArgs()});
return true;
}
@@ -495,7 +534,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
dyn_cast_or_null<CXXMethodDecl>(Callee.Decl))
if (IsFunctor || Method->hasCXXExplicitFunctionObjectParameter())
Args = Args.drop_front(1);
- processCall(Callee, Args);
+ processCall(Callee, E->getRParenLoc(), Args);
return true;
}
@@ -598,6 +637,15 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
}
bool VisitLambdaExpr(LambdaExpr *E) {
+ if (Cfg.InlayHints.LambdaCaptures && E->getCaptureDefault() != LCD_None &&
+ !E->implicit_captures().empty()) {
+ std::string FormattedCaptureList =
+ joinAndTruncate(E->implicit_captures(), Cfg.InlayHints.TypeNameLimit,
+ [](const LambdaCapture &ImplicitCapture) {
+ return getLambdaCaptureName(ImplicitCapture);
+ });
+ addImplicitCaptureHint(E->getCaptureDefaultLoc(), FormattedCaptureList);
+ }
FunctionDecl *D = E->getCallOperator();
if (!E->hasExplicitResultType())
addReturnTypeHint(D, E->hasExplicitParameters()
@@ -709,7 +757,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
private:
using NameVec = SmallVector<StringRef, 8>;
- void processCall(Callee Callee, llvm::ArrayRef<const Expr *> Args) {
+ void processCall(Callee Callee, SourceRange LParenOrBraceRange,
+ llvm::ArrayRef<const Expr *> Args) {
assert(Callee.Decl || Callee.Loc);
if (!Cfg.InlayHints.Parameters || Args.size() == 0)
@@ -721,6 +770,9 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
if (Ctor->isCopyOrMoveConstructor())
return;
+ SmallVector<std::string> FormattedDefaultArgs;
+ bool HasNonDefaultArgs = false;
+
ArrayRef<const ParmVarDecl *> Params, ForwardedParams;
// Resolve parameter packs to their forwarded parameter
SmallVector<const ParmVarDecl *> ForwardedParamsStorage;
@@ -755,12 +807,34 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
bool NameHint = shouldHintName(Args[I], Name);
bool ReferenceHint = shouldHintReference(Params[I], ForwardedParams[I]);
+ bool IsDefault = isa<CXXDefaultArgExpr>(Args[I]);
+ HasNonDefaultArgs |= !IsDefault;
+ if (Cfg.InlayHints.DefaultArguments && IsDefault) {
+ auto SourceText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Params[I]->getDefaultArgRange()),
+ Callee.Decl->getASTContext().getSourceManager(),
+ Callee.Decl->getASTContext().getLangOpts());
+ FormattedDefaultArgs.emplace_back(llvm::formatv(
+ "{0} = {1}", Name,
+ SourceText.size() > Cfg.InlayHints.TypeNameLimit ? "..."
+ : SourceText));
+ }
+
if (NameHint || ReferenceHint) {
addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
InlayHintKind::Parameter, ReferenceHint ? "&" : "",
NameHint ? Name : "", ": ");
}
}
+
+ if (!FormattedDefaultArgs.empty()) {
+ std::string Hint =
+ joinAndTruncate(FormattedDefaultArgs, Cfg.InlayHints.TypeNameLimit,
+ [](const auto &E) { return E; });
+ addInlayHint(LParenOrBraceRange, HintSide::Left,
+ InlayHintKind::DefaultArgument,
+ HasNonDefaultArgs ? ", " : "", Hint, "");
+ }
}
static bool isSetter(const FunctionDecl *Callee, const NameVec &ParamNames) {
@@ -968,6 +1042,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
CHECK_KIND(Type, DeducedTypes);
CHECK_KIND(Designator, Designators);
CHECK_KIND(BlockEnd, BlockEnd);
+ CHECK_KIND(LambdaCapture, LambdaCaptures);
+ CHECK_KIND(DefaultArgument, DefaultArguments);
#undef CHECK_KIND
}
@@ -1021,6 +1097,11 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
/*Prefix=*/"", Text, /*Suffix=*/"=");
}
+ void addImplicitCaptureHint(SourceRange R, llvm::StringRef Text) {
+ addInlayHint(R, HintSide::Right, InlayHintKind::LambdaCapture,
+ /*Prefix=*/": ", Text, /*Suffix=*/"");
+ }
+
bool shouldPrintTypeHint(llvm::StringRef TypeName) const noexcept {
return Cfg.InlayHints.TypeNameLimit == 0 ||
TypeName.size() < Cfg.InlayHints.TypeNameLimit;
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index c08f80442eaa0..dafc3f4eab567 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -209,7 +209,7 @@ bool fromJSON(const llvm::json::Value &Params, ChangeAnnotation &R,
O.map("needsConfirmation", R.needsConfirmation) &&
O.mapOptional("description", R.description);
}
-llvm::json::Value toJSON(const ChangeAnnotation & CA) {
+llvm::json::Value toJSON(const ChangeAnnotation &CA) {
llvm::json::Object Result{{"label", CA.label}};
if (CA.needsConfirmation)
Result["needsConfirmation"] = *CA.needsConfirmation;
@@ -1477,6 +1477,8 @@ llvm::json::Value toJSON(const InlayHintKind &Kind) {
return 2;
case InlayHintKind::Designator:
case InlayHintKind::BlockEnd:
+ case InlayHintKind::LambdaCapture:
+ case InlayHintKind::DefaultArgument:
// This is an extension, don't serialize.
return nullptr;
}
@@ -1517,6 +1519,10 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, InlayHintKind Kind) {
return "designator";
case InlayHintKind::BlockEnd:
return "block-end";
+ case InlayHintKind::LambdaCapture:
+ return "lambda-capture";
+ case InlayHintKind::DefaultArgument:
+ return "default-argument";
}
llvm_unreachable("Unknown clang.clangd.InlayHintKind");
};
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index a0f8b04bc4ffd..793eebf4d51df 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -281,7 +281,7 @@ struct TextDocumentEdit {
/// The text document to change.
VersionedTextDocumentIdentifier textDocument;
- /// The edits to be applied.
+ /// The edits to be applied.
/// FIXME: support the AnnotatedTextEdit variant.
std::vector<TextEdit> edits;
};
@@ -557,7 +557,7 @@ struct ClientCapabilities {
/// The client supports versioned document changes for WorkspaceEdit.
bool DocumentChanges = false;
-
+
/// The client supports change annotations on text edits,
bool ChangeAnnotation = false;
@@ -1013,12 +1013,12 @@ struct WorkspaceEdit {
/// Versioned document edits.
///
/// If a client neither supports `documentChanges` nor
- /// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
- /// using the `changes` property are supported.
+ /// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
+ /// using the `changes` property are supported.
std::optional<std::vector<TextDocumentEdit>> documentChanges;
-
+
/// A map of change annotations that can be referenced in
- /// AnnotatedTextEdit.
+ /// AnnotatedTextEdit.
std::map<std::string, ChangeAnnotation> changeAnnotations;
};
bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path);
@@ -1274,13 +1274,13 @@ enum class InsertTextFormat {
/// Additional details for a completion item label.
struct CompletionItemLabelDetails {
/// An optional string which is rendered less prominently directly after label
- /// without any spacing. Should be used for function signatures or type
+ /// without any spacing. Should be used for function signatures or type
/// annotations.
std::string detail;
/// An optional string which is rendered less prominently after
- /// CompletionItemLabelDetails.detail. Should be used for fully qualified
- /// names or file path.
+ /// CompletionItemLabelDetails.detail. Should be used for fully qualified
+ /// names or file path.
std::string description;
};
llvm::json::Value toJSON(const CompletionItemLabelDetails &);
@@ -1681,6 +1681,21 @@ enum class InlayHintKind {
/// This is a clangd extension.
BlockEnd = 4,
+ /// An inlay hint that is for a variable captured implicitly in a lambda.
+ ///
+ /// An example of parameter hint for implicit lambda captures:
+ /// [&^] { return A; };
+ /// Adds an inlay hint ": A".
+ LambdaCapture = 5,
+
+ /// An inlay hint that is for a default argument.
+ ///
+ /// An example of a parameter hint for a default argument:
+ /// void foo(bool A = true);
+ /// foo(^);
+ /// Adds an inlay hint "A = true".
+ DefaultArgument = 6,
+
/// Other ideas for hints that are not currently implemented:
///
/// * Chaining hints, showing the types of intermediate expressions
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 5b1531eb2fa60..65ab743fb3e38 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -15,9 +15,12 @@
#include "support/Context.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/raw_ostream.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <optional>
#include <string>
+#include <utility>
#include <vector>
namespace clang {
@@ -81,6 +84,8 @@ Config noHintsConfig() {
C.InlayHints.DeducedTypes = false;
C.InlayHints.Designators = false;
C.InlayHints.BlockEnd = false;
+ C.InlayHints.LambdaCaptures = false;
+ C.InlayHints.DefaultArguments = false;
return C;
}
@@ -1458,13 +1463,66 @@ TEST(TypeHints, DefaultTemplateArgs) {
struct A {};
A<float> foo();
auto $var[[var]] = foo();
- A<float> bar[1];
+ A<float> baz;
+ A<float> bar[1]{};
auto [$binding[[value]]] = bar;
)cpp",
ExpectedHint{": A<float>", "var"},
ExpectedHint{": A<float>", "binding"});
}
+TEST(LambdaCaptures, Smoke) {
+ Config Cfg;
+ Cfg.InlayHints.Parameters = false;
+ Cfg.InlayHints.DeducedTypes = false;
+ Cfg.InlayHints.Designators = false;
+ Cfg.InlayHints.BlockEnd = false;
+ Cfg.InlayHints.DefaultArguments = false;
+
+ Cfg.InlayHints.LambdaCaptures = true;
+ Cfg.InlayHints.TypeNameLimit = 10;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ assertHints(InlayHintKind::LambdaCapture, R"cpp(
+ void foo() {
+ int A = 1;
+ int ReallyLongName = 1;
+ auto B = [$byvalue[[=]]] { return A; };
+ auto C = [$byref[[&]]] { return A; };
+ auto D = [$trunc[[=]], &A] { B(); (void)ReallyLongName; return A; };
+ }
+ )cpp",
+ ExpectedHint{": A", "byvalue", Right},
+ ExpectedHint{": A", "byref", Right},
+ ExpectedHint{": B, ...", "trunc", Right});
+}
+
+TEST(DefaultArguments, Smoke) {
+ Config Cfg;
+ Cfg.InlayHints.Parameters =
+ true; // To test interplay of parameters and default parameters
+ Cfg.InlayHints.DeducedTypes = false;
+ Cfg.InlayHints.Designators = false;
+ Cfg.InlayHints.BlockEnd = false;
+ Cfg.InlayHints.LambdaCaptures = false;
+
+ Cfg.InlayHints.DefaultArguments = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ const auto *Code = R"cpp(
+ int foo(int A = 4) { return A; }
+ int bar(int A, int B = 1, bool C = foo($default1[[)]]) { return A; }
+ int A = bar($explicit[[2]]$default2[[)]];
+ )cpp";
+
+ assertHints(InlayHintKind::DefaultArgument, Code,
+ ExpectedHint{"A = 4", "default1", Left},
+ ExpectedHint{", B = 1, C = foo()", "default2", Left});
+
+ assertHints(InlayHintKind::Parameter, Code,
+ ExpectedHint{"A: ", "explicit", Left});
+}
+
TEST(TypeHints, Deduplication) {
assertTypeHints(R"cpp(
template <typename T>
@@ -1568,7 +1626,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
)cpp";
llvm::StringRef VectorIntPtr = R"cpp(
- vector<int *> array;
+ vector<int *> $init[[array]];
auto $no_modifier[[x]] = array[3];
auto* $ptr_modifier[[ptr]] = &array[3];
auto& $ref_modifier[[ref]] = array[3];
@@ -1592,7 +1650,7 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
ExpectedHint{": non_template_iterator", "end"});
llvm::StringRef VectorInt = R"cpp(
- vector<int> array;
+ vector<int> $init[[array]];
auto $no_modifier[[by_value]] = array[3];
auto* $ptr_modifier[[ptr]] = &array[3];
auto& $ref_modifier[[ref]] = array[3];
@@ -1741,7 +1799,7 @@ TEST(ParameterHints, PseudoObjectExpr) {
int printf(const char *Format, ...);
int main() {
- S s;
+ S $init[[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
|
zyn0217
reviewed
Jun 17, 2024
There was a problem hiding this comment.
My two cents:
While I appreciate the value of inspecting more semantic information about lambdas, I still have mixed feelings about user experience.
Admittedly, presenting implicitly captured variables provides users with insight into ODR-used variables, but I'm unsure about our strategy: We would probably end up showing too many hints inside the lambda capture, which would visually elongate the lambda header and distract users from reading.
Indeed, we have a configuration entry to restrict the length, but I feel that's insufficient (Although we're baking it with TypeNameLimit currently, which is also inappropriate). For example, a user might have a lambda capturing several variables named in single letters e.g. a-z. As a result, we probably eventually display [&: a, b, c, d, e, f, g, ...](...), which is IMO too spammy.
That said, I wonder if we can implement the capture stuff in hovers instead. From what I can think of, we present something e.g.
// implicitly captured by reference/copy:
int a;
std::string s;
...when user hovers over the capture defaults.
(I don't have a strong opinion on the hints for default arguments. It would also be great to hear @HighCommander4 and @kadircet's input.)
Comment on lines
396
to
410
| std::string Out; | ||
| bool IsFirst = true; | ||
| for (auto &&Element : Range) { | ||
| if (!IsFirst) | ||
| Out.append(", "); | ||
| else | ||
| IsFirst = false; |
There was a problem hiding this comment.
nit: 1. We don't usually append to a string; instead, we use llvm::raw_string_ostream. 2. This whole thing can be simplified by llvm::ListSeparator
There was a problem hiding this comment.
Changed this to use raw_string_ostream, lmk how it looks
|
Aha, that's a great suggestion. I agree that beyond the trivial case of 2 captured variables it gets a bit noisy. On-hover would be quite nice though! To clarify, you mean hovering over the default capture '=' or '&' right? I'm happy to remove the lambdas from this PR in favor of the hover approach. Thoughts on the default arguments? |
Yep, that's what I thought.
No worries, but let's wait for other reviewers' opinions before we move forward. :) |
torshepherd
force-pushed
the
clangd-subset-of-inlay-hints
branch
4 times, most recently
from
c856574
to
bd294be
Compare
|
Ok I took a stab at doing lambda captures as hover instead.
It strikes me, if we could either add HoverInfo or styling to the variables within the body of the lambda themselves, that would be the most readable representation probably. For instance:
Overall I think capture hints should be removed from this PR so we can get default args in in the meantime. |
torshepherd
force-pushed
the
clangd-subset-of-inlay-hints
branch
from
bd294be
to
f7a243b
Compare
HighCommander4
requested changes
Sep 3, 2024
There was a problem hiding this comment.
Apologies for the slow response time, it's been quite a busy summer.
High-level feedback:
- Let's definitely split the default arguments part from the lambda captures part. Even if we decide we want both, it helps to have separate features in separate patches, so that e.g. if one causes a problem and needs to be reverted, it's easy to do that without affecting the other.
- I tend to agree with @zyn0217 that lambda captures might be a better fit for hovers than inlay hints. I think we could solve the problem targeting the capture-default token: if
SelectionTreeselects theLambdaExpr, massage the input position into aSourceLocation, compare it togetCaptureDefaultLoc(), and if so then take a different branch (inHover.cppcode). (This doesn't prevent us from potentially also pursuing the idea of adding a "captures" section to the hover for the lambda variable.) - For the default argument hints, what do you think about using
name: valuesyntax instead ofname = valuesyntax, for consistency with parameter hints?
torshepherd
changed the title
torshepherd
force-pushed
the
clangd-subset-of-inlay-hints
branch
3 times, most recently
from
41f0a7f
to
2c9e7c1
Compare
|
Hmm another case to consider:
In this case the argument is unnamed. I think for this I should just insert |
Fixed this and added a test case
|
|
Ping |
|
Ok I found a couple of nontrivial issues that I took care of in the latest commit.
Also, a couple of follow-up things I should do:
struct Foo { int a; int b = 2 };
Foo f{1}; // Currently shows 'Foo f{.a=1}', should show 'Foo f{.a=1, .b=2}'
|
That sort of diagnostic is likely produced by a YAML plugin, which uses a schema from https://github.com/SchemaStore/schemastore. I do file a SchemaStore issue once per clangd release about updating the clangd schema, here's the one for clangd 19: SchemaStore/schemastore#4022. If you'd like to file one about |
HighCommander4
approved these changes
Oct 18, 2024
torshepherd
force-pushed
the
clangd-subset-of-inlay-hints
branch
from
de95459
to
808f9f4
Compare
|
Accepted the comment and added relnotes. Lmk if you need anything else prior to merge |
|
Thanks. The buildkite failure looks unrelated (it's a |
|
@torshepherd Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Title. This PR adds support for defaulted function arguments:
as inlay hints.
If the list of default args is too long (based on TypeNameLimit), shows "..." instead:
Note: I wrote this before #85497 landed. With this we should probably also follow up with go-to-definition on the lambda captures at least; I could see that being helpful
Edit (9/23): Removed ' and implicit lambda captures'