daiyousei-qz updated this revision to Diff 522898.
daiyousei-qz marked 16 inline comments as done.
daiyousei-qz added a comment.
- addressed many review comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150635/new/
https://reviews.llvm.org/D150635
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/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
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
@@ -77,6 +77,7 @@
C.InlayHints.Parameters = false;
C.InlayHints.DeducedTypes = false;
C.InlayHints.Designators = false;
+ C.InlayHints.EndDefinitionComments = false;
return C;
}
@@ -122,6 +123,17 @@
assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
}
+template <typename... ExpectedHints>
+void assertEndDefinitionHints(llvm::StringRef AnnotatedSource,
+ uint32_t MinLines, ExpectedHints... Expected) {
+ Config Cfg;
+ Cfg.InlayHints.EndDefinitionComments = true;
+ Cfg.InlayHints.EndDefinitionCommentMinLines = MinLines;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+ assertHints(InlayHintKind::EndDefinitionComment, AnnotatedSource,
+ Expected...);
+}
+
TEST(ParameterHints, Smoke) {
assertParameterHints(R"cpp(
void foo(int param);
@@ -1550,6 +1562,196 @@
ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
ExpectedHint{"c: ", "param3"});
}
+
+TEST(EndDefinitionHints, Functions) {
+ assertEndDefinitionHints(R"cpp(
+ $foo[[int foo() {
+ return 41;
+ }]]
+
+ template<int X>
+ $bar[[int bar() { return X; }]]
+
+ // No hint because this isn't a definition
+ int buz();
+ )cpp",
+ 0, ExpectedHint{" // foo", "foo"},
+ ExpectedHint{" // bar", "bar"});
+}
+
+TEST(EndDefinitionHints, Methods) {
+ assertEndDefinitionHints(R"cpp(
+ class Test {
+ public:
+ // No hint because there's no function body
+ Test() = default;
+
+ $dtor[[~Test() {}]]
+
+ $method1[[void method1() {}]]
+
+ // No hint because this isn't a definition
+ void method2();
+
+ template <typename T>
+ $method3[[void method3() {}]]
+
+ // No hint because this isn't a definition
+ template <typename T>
+ void method4();
+ } x;
+
+ $method2[[void Test::method2() {}]]
+
+ template <typename T>
+ $method4[[void Test::method4() {}]]
+ )cpp",
+ 0, ExpectedHint{" // ~Test", "dtor"},
+ ExpectedHint{" // method1", "method1"},
+ ExpectedHint{" // method3", "method3"},
+ ExpectedHint{" // Test::method2", "method2"},
+ ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+ assertEndDefinitionHints(R"cpp(
+ struct S {
+ $opId[[S operator+(int) const {
+ return *this;
+ }]]
+
+ $opBool[[operator bool() const {
+ return true;
+ }]]
+
+ // No hint because there's no function body
+ operator int() const = delete;
+
+ // No hint because this isn't a definition
+ operator float() const;
+ } x;
+
+ $opEq[[bool operator==(const S&, const S&) {
+ return true;
+ }]]
+ )cpp",
+ 0, ExpectedHint{" // operator+", "opId"},
+ ExpectedHint{" // operator bool", "opBool"},
+ ExpectedHint{" // operator==", "opEq"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+ assertEndDefinitionHints(
+ R"cpp(
+ $anon[[namespace {
+ void foo();
+ }]]
+
+ $ns[[namespace ns {
+ void bar();
+ }]]
+ )cpp",
+ 0, ExpectedHint{" // namespace", "anon"},
+ ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(EndDefinitionHints, Types) {
+ assertEndDefinitionHints(
+ R"cpp(
+ $S[[struct S {
+ };]]
+
+ $C[[class C {
+ };]]
+
+ $U[[union U {
+ };]]
+
+ $E1[[enum E1 {
+ };]]
+
+ $E2[[enum class E2 {
+ };]]
+ )cpp",
+ 0, ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+ ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+ ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(EndDefinitionHints, BundledTypeVariableDecl) {
+ assertEndDefinitionHints(
+ R"cpp(
+ // No hint because we have a declarator right after '}'
+ struct {
+ int x;
+ } s;
+
+ // Rare case, but yes we'll have a hint here.
+ $anon[[struct {
+ int x;
+ }]]
+
+ s2;
+ )cpp",
+ 0, ExpectedHint{" // struct", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+ assertEndDefinitionHints(R"cpp(
+ // The hint is placed after the trailing ';'
+ $S1[[struct S1 {
+ } ;]]
+
+ // The hint is always placed in the same line with the closing '}'.
+ // So in this case where ';' is missing, it is attached to '}'.
+ $S2[[struct S2 {
+ }]]
+
+ ;
+
+ // No hint because only one trailing ';' is allowed
+ struct S3 {
+ };;
+
+ // No hint becaus trailing ';' is only allowed for class/struct/union/enum
+ void foo() {
+ };
+ )cpp",
+ 0, ExpectedHint{" // struct S1", "S1"},
+ ExpectedHint{" // struct S2", "S2"});
+}
+
+TEST(EndDefinitionHints, TrailingText) {
+ assertEndDefinitionHints(R"cpp(
+ $S1[[struct S1 {
+ } ;]]
+
+ // No hint for S2 because of the trailing comment
+ struct S2 {
+ }; /* Put anything here */
+
+ // No hint for S3 because of the trailing source code
+ struct S3 {}; $S4[[struct S4 {};]]
+
+ // No hint for ns because of the trailing comment
+ namespace ns {
+
+ } // namespace ns
+ )cpp",
+ 0, ExpectedHint{" // struct S1", "S1"},
+ ExpectedHint{" // struct S4", "S4"});
+}
+
+TEST(EndDefinitionHints, MinLineConfig) {
+ assertEndDefinitionHints(R"cpp(
+ struct S1 {};
+
+ $S2[[struct S2 {
+ };]]
+ )cpp",
+ 2, ExpectedHint{" // struct S2", "S2"});
+}
+
// FIXME: Low-hanging fruit where we could omit a type hint:
// - auto x = TypeName(...);
// - auto x = (TypeName) (...);
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -236,6 +236,8 @@
InlayHints:
Enabled: No
ParameterNames: Yes
+ EndDefinitionComments: Yes
+ EndDefinitionCommentMinLines: 10
)yaml");
auto Results =
Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
@@ -244,6 +246,10 @@
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, std::nullopt);
+ EXPECT_THAT(Results[0].InlayHints.EndDefinitionComments,
+ llvm::ValueIs(val(true)));
+ EXPECT_THAT(Results[0].InlayHints.EndDefinitionCommentMinLines,
+ llvm::ValueIs(val(10)));
}
TEST(ParseYAML, IncludesIgnoreHeader) {
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -1647,6 +1647,16 @@
/// This is a clangd extension.
Designator = 3,
+ /// A hint after function, type or namespace definition, indicating the
+ /// defined symbol name of the definition.
+ ///
+ /// An example of a decl name hint in this position:
+ /// void func() {
+ /// } ^
+ /// Uses comment-like syntax like "/* func */".
+ /// This is a clangd extension.
+ EndDefinitionComment = 4,
+
/// Other ideas for hints that are not currently implemented:
///
/// * Chaining hints, showing the types of intermediate expressions
Index: clang-tools-extra/clangd/Protocol.cpp
===================================================================
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -1435,6 +1435,8 @@
case InlayHintKind::Parameter:
return 2;
case InlayHintKind::Designator: // This is an extension, don't serialize.
+ case InlayHintKind::EndDefinitionComment: // This is an extension, don't
+ // serialize.
return nullptr;
}
llvm_unreachable("Unknown clang.clangd.InlayHintKind");
@@ -1468,6 +1470,8 @@
return "type";
case InlayHintKind::Designator:
return "designator";
+ case InlayHintKind::EndDefinitionComment:
+ return "end-definition-comment";
}
llvm_unreachable("Unknown clang.clangd.InlayHintKind");
};
Index: clang-tools-extra/clangd/InlayHints.cpp
===================================================================
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -274,6 +274,42 @@
addReturnTypeHint(D, FTL.getRParenLoc());
}
}
+ if (Cfg.InlayHints.EndDefinitionComments &&
+ D->isThisDeclarationADefinition()) {
+ addEndDefinitionCommentHint(*D, "", false);
+ }
+ return true;
+ }
+
+ bool VisitEnumDecl(EnumDecl *D) {
+ if (Cfg.InlayHints.EndDefinitionComments &&
+ D->isThisDeclarationADefinition()) {
+ StringRef DeclPrefix;
+ if (!D->isScoped()) {
+ DeclPrefix = "enum";
+ } else if (D->isScopedUsingClassTag()) {
+ DeclPrefix = "enum class";
+ } else {
+ DeclPrefix = "enum struct";
+ }
+
+ addEndDefinitionCommentHint(*D, DeclPrefix, true);
+ }
+ return true;
+ }
+
+ bool VisitRecordDecl(RecordDecl *D) {
+ if (Cfg.InlayHints.EndDefinitionComments &&
+ D->isThisDeclarationADefinition()) {
+ addEndDefinitionCommentHint(*D, D->getKindName(), true);
+ }
+ return true;
+ }
+
+ bool VisitNamespaceDecl(NamespaceDecl *D) {
+ if (Cfg.InlayHints.EndDefinitionComments) {
+ addEndDefinitionCommentHint(*D, "namespace", false);
+ }
return true;
}
@@ -539,6 +575,36 @@
return SourcePrefix.endswith("/*");
}
+ // Check if the `D` has no trailing characters after the last line of the
+ // definition, except for whitespace and possibly a ';'. Since the hint should
+ // be displayed after the trailing whitespaces, the number of characters to
+ // skip is returned through `SkipChars`
+ bool shouldHintEndDefinitionComment(const NamedDecl &D, bool SkipSemicolon,
+ size_t &SkipChars) {
+ auto &SM = AST.getSourceManager();
+ auto FileLoc = SM.getFileLoc(D.getEndLoc());
+ auto Decomposed = SM.getDecomposedLoc(FileLoc);
+ if (Decomposed.first != MainFileID)
+ return false;
+
+ StringRef Whitespaces = " \t\v";
+ StringRef DeclRangeSuffix = MainFileBuf.substr(Decomposed.second);
+ if (!DeclRangeSuffix.consume_front("}"))
+ return false;
+
+ // By default, the hint is attached to '}'
+ SkipChars = 0;
+ StringRef HintRangeSuffix = DeclRangeSuffix.ltrim(Whitespaces);
+ if (SkipSemicolon && HintRangeSuffix.consume_front(";")) {
+ // Attach the hint to ';'
+ SkipChars = DeclRangeSuffix.size() - HintRangeSuffix.size();
+ HintRangeSuffix = HintRangeSuffix.ltrim(Whitespaces);
+ }
+
+ return HintRangeSuffix.empty() || HintRangeSuffix.starts_with("\n") ||
+ HintRangeSuffix.starts_with("\r\n");
+ };
+
// If "E" spells a single unqualified identifier, return that name.
// Otherwise, return an empty string.
static StringRef getSpelledIdentifier(const Expr *E) {
@@ -616,6 +682,16 @@
void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
llvm::StringRef Prefix, llvm::StringRef Label,
llvm::StringRef Suffix) {
+ auto LSPRange = getHintRange(R);
+ if (!LSPRange)
+ return;
+
+ addInlayHint(*LSPRange, Side, Kind, Prefix, Label, Suffix);
+ }
+
+ void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+ llvm::StringRef Prefix, llvm::StringRef Label,
+ llvm::StringRef Suffix) {
// We shouldn't get as far as adding a hint if the category is disabled.
// We'd like to disable as much of the analysis as possible above instead.
// Assert in debug mode but add a dynamic check in production.
@@ -631,20 +707,18 @@
CHECK_KIND(Parameter, Parameters);
CHECK_KIND(Type, DeducedTypes);
CHECK_KIND(Designator, Designators);
+ CHECK_KIND(EndDefinitionComment, EndDefinitionComments);
#undef CHECK_KIND
}
- auto LSPRange = getHintRange(R);
- if (!LSPRange)
- return;
- Position LSPPos = Side == HintSide::Left ? LSPRange->start : LSPRange->end;
+ Position LSPPos = Side == HintSide::Left ? LSPRange.start : LSPRange.end;
if (RestrictRange &&
(LSPPos < RestrictRange->start || !(LSPPos < RestrictRange->end)))
return;
bool PadLeft = Prefix.consume_front(" ");
bool PadRight = Suffix.consume_back(" ");
Results.push_back(InlayHint{LSPPos, (Prefix + Label + Suffix).str(), Kind,
- PadLeft, PadRight, *LSPRange});
+ PadLeft, PadRight, LSPRange});
}
// Get the range of the main file that *exactly* corresponds to R.
@@ -699,6 +773,41 @@
/*Prefix=*/"", Text, /*Suffix=*/"=");
}
+ void addEndDefinitionCommentHint(const NamedDecl &D, StringRef DeclPrefix,
+ bool SkipSemicolon) {
+ size_t SkippedChars = 0;
+ if (!shouldHintEndDefinitionComment(D, SkipSemicolon, SkippedChars))
+ return;
+
+ // Note this range doesn't include the trailing ';' in type definitions.
+ // So we have to add SkippedChars to the end character.
+ SourceRange R = D.getSourceRange();
+ auto LSPRange = getHintRange(R);
+ if (!LSPRange ||
+ static_cast<uint32_t>(LSPRange->end.line - LSPRange->start.line) + 1 <
+ Cfg.InlayHints.EndDefinitionCommentMinLines)
+ return;
+
+ LSPRange->end.character += SkippedChars;
+
+ std::string Label = DeclPrefix.str();
+ if (const auto *FD = dyn_cast_or_null<FunctionDecl>(&D)) {
+ // `getSimpleName` cannot handle operator overloading, so we handle it
+ // differently.
+ assert(Label.empty());
+ Label += printName(AST, D);
+ } else {
+ StringRef Name = getSimpleName(D);
+ if (!Name.empty()) {
+ Label += ' ';
+ Label += Name;
+ }
+ }
+
+ addInlayHint(*LSPRange, HintSide::Right,
+ InlayHintKind::EndDefinitionComment, " // ", Label, "");
+ }
+
std::vector<InlayHint> &Results;
ASTContext &AST;
const syntax::TokenBuffer &Tokens;
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -254,10 +254,18 @@
if (auto Value = boolValue(N, "Designators"))
F.Designators = *Value;
});
+ Dict.handle("EndDefinitionComments", [&](Node &N) {
+ if (auto Value = boolValue(N, "EndDefinitionComments"))
+ F.EndDefinitionComments = *Value;
+ });
Dict.handle("TypeNameLimit", [&](Node &N) {
if (auto Value = uint32Value(N, "TypeNameLimit"))
F.TypeNameLimit = *Value;
});
+ Dict.handle("EndDefinitionCommentMinLines", [&](Node &N) {
+ if (auto Value = uint32Value(N, "EndDefinitionCommentMinLines"))
+ F.EndDefinitionCommentMinLines = *Value;
+ });
Dict.parse(N);
}
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -322,8 +322,12 @@
std::optional<Located<bool>> DeducedTypes;
/// Show designators in aggregate initialization.
std::optional<Located<bool>> Designators;
+ /// Show defined symbol names at the end of a definition.
+ std::optional<Located<bool>> EndDefinitionComments;
/// Limit the length of type name hints. (0 means no limit)
std::optional<Located<uint32_t>> TypeNameLimit;
+ ///
+ std::optional<Located<uint32_t>> EndDefinitionCommentMinLines;
};
InlayHintsBlock InlayHints;
};
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -611,11 +611,21 @@
Out.Apply.push_back([Value(**F.Designators)](const Params &, Config &C) {
C.InlayHints.Designators = Value;
});
+ if (F.EndDefinitionComments)
+ Out.Apply.push_back(
+ [Value(**F.EndDefinitionComments)](const Params &, Config &C) {
+ C.InlayHints.EndDefinitionComments = Value;
+ });
if (F.TypeNameLimit)
Out.Apply.push_back(
[Value(**F.TypeNameLimit)](const Params &, Config &C) {
C.InlayHints.TypeNameLimit = Value;
});
+ if (F.EndDefinitionCommentMinLines)
+ Out.Apply.push_back(
+ [Value(**F.EndDefinitionCommentMinLines)](const Params &, Config &C) {
+ C.InlayHints.EndDefinitionCommentMinLines = Value;
+ });
}
constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -147,8 +147,13 @@
bool Parameters = true;
bool DeducedTypes = true;
bool Designators = true;
+ bool EndDefinitionComments = true;
// Limit the length of type names in inlay hints. (0 means no limit)
uint32_t TypeNameLimit = 32;
+
+ // The minimal number of lines of the definition to show the
+ // end-definition comment hints.
+ uint32_t EndDefinitionCommentMinLines = 2;
} InlayHints;
};
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
