[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e6a342fdac8: [clangd] Implement end-definition-comment 
inlay hints (authored by daiyousei-qz, committed by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D150635?vs=525922=540750#toc

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/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.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1629,6 +1639,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Oh no, I just saw this never landed - I suspect you were waiting for me to 
commit it, and I was expecting you to do it!

I've rebased and I'll commit it for you now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.

Addressed the remaining comments. Thanks everyone for helping review and 
improve this patch!




Comment at: clang-tools-extra/clangd/InlayHints.cpp:802
+Position HintStart = sourceLocToPosition(SM, RBraceLoc);
+Position HintEnd = {HintStart.line,
+HintStart.character +

sammccall wrote:
> I'd prefer `sourceLocToPosition(SM, 
> RBraceLoc.getLocationWithOffset(HintRangeText.size()))` which would avoids 
> such low-level conversion between coordinate systems, and seems to perform 
> just fine (we're going to hit SourceManager's caches).
> 
> Will leave this up to you though.
Yeah, this is definitely clearer. Updated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525922.
daiyousei-qz marked 8 inline comments as done.
daiyousei-qz added a comment.

- Address remaining comments


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/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.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for class/struct/union/enum
+void foo() {
+};
+
+// Rare case, but yes we'll have a hint here.
+struct {
+

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

> 150ms is more than noticeable

Fair enough. I also see we *are* avoiding the quadratic algorithm in other 
places in inlay hints, and that we can't easily reuse the getHintRange() 
function in any case without switching to handling tokens instead of strings. 
So overall I think the current version is fine. Thanks for digging into this!




Comment at: clang-tools-extra/clangd/Config.h:150
 bool Designators = true;
+bool BlockEnd = true;
 // Limit the length of type names in inlay hints. (0 means no limit)

this needs to be false initially (a month or so?) so we have a chance to try it 
out in practice, ensure it's not too spammy/has crashes etc, then we can flip 
on by default



Comment at: clang-tools-extra/clangd/InlayHints.cpp:802
+Position HintStart = sourceLocToPosition(SM, RBraceLoc);
+Position HintEnd = {HintStart.line,
+HintStart.character +

I'd prefer `sourceLocToPosition(SM, 
RBraceLoc.getLocationWithOffset(HintRangeText.size()))` which would avoids such 
low-level conversion between coordinate systems, and seems to perform just fine 
(we're going to hit SourceManager's caches).

Will leave this up to you though.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

daiyousei-qz wrote:
> nridge wrote:
> > daiyousei-qz wrote:
> > > sammccall wrote:
> > > > We actually already have a configurable inlay hint length limit.
> > > > It has the wrong name (`TypeNameLimit`) , but I think we should use it 
> > > > anyway (and maybe try to fix the name later).
> > > > 
> > > > (I'm not sure making this configurable was a great idea, but given that 
> > > > it exists, people will expect it to effectively limit the length of 
> > > > hints)
> > > `TypeNameLimit` was introduced to limit the length of "DeducedTypes" 
> > > only. I don't think using that number for all hints is a good idea 
> > > because different hints appear in different contexts. For deduced type 
> > > hint, it is displayed in the middle of the actual code, so it must be 
> > > concise to not overwhelm the actual code. However, this hint is usually 
> > > displayed at the end of an almost empty line. A much larger number of 
> > > characters should be allowed.
> > > 
> > > (I'm not arguing here, but IMO such options are definitely helpful. Not 
> > > everybody would fork llvm-project and build their own version of clangd 
> > > binary. Without options to configure length of hints, people would 
> > > disable "DeducedTypes" entirely if they cannot tolerate the default 
> > > length limit, while this feature is definitely a cool thing to turn on. 
> > > Personally, I actually think clangd has too little option compared to say 
> > > rust-analyzer. But it's just my understanding)
> > > For deduced type hint, it is displayed in the middle of the actual code, 
> > > so it must be concise to not overwhelm the actual code. However, this 
> > > hint is usually displayed at the end of an almost empty line. A much 
> > > larger number of characters should be allowed.
> > 
> > Another consideration besides the location of the hint that is worth 
> > keeping in mind, is what type of content is being printed.
> > 
> > Type names in C++ are composable in ways that identifiers are not (e.g. 
> > `vector, allocator, 
> > allocator>` etc.), such that there is a need to limit 
> > type hints that doesn't really apply to say, parameter hints.
> > 
> > So, a question I have here is: can template argument lists appear in an 
> > end-definition-comment hint?
> > 
> > For example, for:
> > 
> > ```
> > template 
> > struct S {
> >   void foo();
> > };
> > 
> > template 
> > void S::foo() {
> > }  // <--- HERE
> > ```
> > 
> > is the hint at the indicated location `S::foo()`, or `S::foo()`? In the 
> > latter case, we can imagine the hint getting long due to the type 
> > parameters, and we may want to consider either limiting its length, or 
> > tweaking the code to not print the type parameters.
> Yes, currently this hint is only shown if the total length of hint label is 
> less than 60 characters. I believe what we display should be exactly what is 
> used to define the symbol. In your example,
> 
> ```
> template 
> struct S {
>   void foo();
> };
> 
> template 
> void S::foo() {
> }  // S::foo
> ```
> Basically, "[A]" and "[B]" is the same text (OFC excluding whitespaces).
> ```
> void [A]() {
> } // [B]
> ```
> The hint won't be displayed if "[B]" is more than 60 characters.
OK, I'm sold on wanting a higher limit here, and a hard-coded 60 as a limit on 
the hint seems simple and reasonable for now. We can iterate on this.

(There's another case with arbitrary types being printed: `operator 
vector` or so)

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

nridge wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > We actually already have a configurable inlay hint length limit.
> > > It has the wrong name (`TypeNameLimit`) , but I think we should use it 
> > > anyway (and maybe try to fix the name later).
> > > 
> > > (I'm not sure making this configurable was a great idea, but given that 
> > > it exists, people will expect it to effectively limit the length of hints)
> > `TypeNameLimit` was introduced to limit the length of "DeducedTypes" only. 
> > I don't think using that number for all hints is a good idea because 
> > different hints appear in different contexts. For deduced type hint, it is 
> > displayed in the middle of the actual code, so it must be concise to not 
> > overwhelm the actual code. However, this hint is usually displayed at the 
> > end of an almost empty line. A much larger number of characters should be 
> > allowed.
> > 
> > (I'm not arguing here, but IMO such options are definitely helpful. Not 
> > everybody would fork llvm-project and build their own version of clangd 
> > binary. Without options to configure length of hints, people would disable 
> > "DeducedTypes" entirely if they cannot tolerate the default length limit, 
> > while this feature is definitely a cool thing to turn on. Personally, I 
> > actually think clangd has too little option compared to say rust-analyzer. 
> > But it's just my understanding)
> > For deduced type hint, it is displayed in the middle of the actual code, so 
> > it must be concise to not overwhelm the actual code. However, this hint is 
> > usually displayed at the end of an almost empty line. A much larger number 
> > of characters should be allowed.
> 
> Another consideration besides the location of the hint that is worth keeping 
> in mind, is what type of content is being printed.
> 
> Type names in C++ are composable in ways that identifiers are not (e.g. 
> `vector, allocator, 
> allocator>` etc.), such that there is a need to limit type 
> hints that doesn't really apply to say, parameter hints.
> 
> So, a question I have here is: can template argument lists appear in an 
> end-definition-comment hint?
> 
> For example, for:
> 
> ```
> template 
> struct S {
>   void foo();
> };
> 
> template 
> void S::foo() {
> }  // <--- HERE
> ```
> 
> is the hint at the indicated location `S::foo()`, or `S::foo()`? In the 
> latter case, we can imagine the hint getting long due to the type parameters, 
> and we may want to consider either limiting its length, or tweaking the code 
> to not print the type parameters.
Yes, currently this hint is only shown if the total length of hint label is 
less than 60 characters. I believe what we display should be exactly what is 
used to define the symbol. In your example,

```
template 
struct S {
  void foo();
};

template 
void S::foo() {
}  // S::foo
```
Basically, "[A]" and "[B]" is the same text (OFC excluding whitespaces).
```
void [A]() {
} // [B]
```
The hint won't be displayed if "[B]" is more than 60 characters.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

daiyousei-qz wrote:
> sammccall wrote:
> > We actually already have a configurable inlay hint length limit.
> > It has the wrong name (`TypeNameLimit`) , but I think we should use it 
> > anyway (and maybe try to fix the name later).
> > 
> > (I'm not sure making this configurable was a great idea, but given that it 
> > exists, people will expect it to effectively limit the length of hints)
> `TypeNameLimit` was introduced to limit the length of "DeducedTypes" only. I 
> don't think using that number for all hints is a good idea because different 
> hints appear in different contexts. For deduced type hint, it is displayed in 
> the middle of the actual code, so it must be concise to not overwhelm the 
> actual code. However, this hint is usually displayed at the end of an almost 
> empty line. A much larger number of characters should be allowed.
> 
> (I'm not arguing here, but IMO such options are definitely helpful. Not 
> everybody would fork llvm-project and build their own version of clangd 
> binary. Without options to configure length of hints, people would disable 
> "DeducedTypes" entirely if they cannot tolerate the default length limit, 
> while this feature is definitely a cool thing to turn on. Personally, I 
> actually think clangd has too little option compared to say rust-analyzer. 
> But it's just my understanding)
> For deduced type hint, it is displayed in the middle of the actual code, so 
> it must be concise to not overwhelm the actual code. However, this hint is 
> usually displayed at the end of an almost empty line. A much larger number of 
> characters should be allowed.

Another consideration besides the location of the hint that is worth keeping in 
mind, is what type of content is being printed.

Type names in C++ are composable in ways that identifiers are not (e.g. 
`vector, allocator, 
allocator>` etc.), such that there is a need to limit type 
hints that doesn't really apply to say, parameter hints.

So, a question I have here is: can template argument lists appear in an 
end-definition-comment hint?

For example, for:

```
template 
struct S {
  void foo();
};

template 
void S::foo() {
}  // <--- HERE
```

is the hint at the indicated location `S::foo()`, or `S::foo()`? In the 
latter case, we can imagine the hint getting long due to the type parameters, 
and we may want to consider either limiting its length, or tweaking the code to 
not print the type parameters.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Addressed review comments except for those we don't have an agreement yet.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

sammccall wrote:
> We actually already have a configurable inlay hint length limit.
> It has the wrong name (`TypeNameLimit`) , but I think we should use it anyway 
> (and maybe try to fix the name later).
> 
> (I'm not sure making this configurable was a great idea, but given that it 
> exists, people will expect it to effectively limit the length of hints)
`TypeNameLimit` was introduced to limit the length of "DeducedTypes" only. I 
don't think using that number for all hints is a good idea because different 
hints appear in different contexts. For deduced type hint, it is displayed in 
the middle of the actual code, so it must be concise to not overwhelm the 
actual code. However, this hint is usually displayed at the end of an almost 
empty line. A much larger number of characters should be allowed.

(I'm not arguing here, but IMO such options are definitely helpful. Not 
everybody would fork llvm-project and build their own version of clangd binary. 
Without options to configure length of hints, people would disable 
"DeducedTypes" entirely if they cannot tolerate the default length limit, while 
this feature is definitely a cool thing to turn on. Personally, I actually 
think clangd has too little option compared to say rust-analyzer. But it's just 
my understanding)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:597
+auto BlockBeginLine =
+SM.getSpellingLineNumber(BraceRange.getBegin(), );
+if (Invalid)

sammccall wrote:
> this should be getFileLoc(BraceRange.getBegin()), I think?
Thanks for pointing out! As per comments below, calling `getLineNumber` instead 
of `getSpellingLineNumber` in the latest version.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// 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();

sammccall wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > This is too much arithmetic and fiddly coupling between this function and 
> > > shouldHintEndDefinitionComment.
> > > 
> > > Among other things, it allows for confusion between unicode characters 
> > > (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And 
> > > we do have a bug here: shouldHintEndDefinition provides SkippedChars in 
> > > clang bytes, but you add it to end.character below which is in LSP 
> > > characters.
> > > 
> > > ---
> > > 
> > > Let's redesign a little... 
> > > 
> > > We have a `}` on some line. We want to compute a sensible part of that 
> > > line to attach to.
> > > A suitable range may not exist, in which case we're going to omit the 
> > > hint.
> > > 
> > > - The line consists of text we don't care about , the `}`, and then some 
> > > mix of whitespace, "trivial" punctuation, and "nontrivial" chars.
> > > - the range should always start at the `}`, since that's what we're 
> > > really hinting
> > > - to get the hint in the right place, the range should end after the 
> > > trivial chars, but before any trailing whitespace
> > > - if there are any nontrivial chars, there's no suitable range
> > > 
> > > so something like:
> > > 
> > > ```
> > > optional findBraceTargetRange(SourceLocation CloseBrace) {
> > >   auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
> > >   if (File != MainFileID) return std::nullopt;
> > >   StringRef RestOfLine = 
> > > MainFileBuf.substr(Offset).split('\n').first.rtrim();
> > >   if (!RestOfLine.consume_front("{")) return;
> > >   if (StringRef::npos != Punctuation.find_first_of(" ,;")) return 
> > > std::nullopt;
> > >   return {offsetToPosition(MainFileBuf, Offset), 
> > > offsetToPosition(MainFileBuf, Result.bytes_end() - 
> > > MainFileBuf.bytes_end()};
> > > }
> > > ```
> > > 
> > > and then call from addEndDefinitionComment, the result is LSPRange 
> > > already.
> > Done, also moved min-line and max-length logic to this function. Btw, I 
> > think we should avoid `offsetToPosition` as much as possible. It is a large 
> > overhead considering inlay hints are recomputed throughout the entire file 
> > for each edit. I frequently work with source code that's nearly 1MB large 
> > (Yes, I don't think we should have source file this large, but it is what 
> > it is).
> > also moved min-line and max-length logic to this function
> 
> I'd prefer you to move them back. As specified, that function is otherwise 
> strictly about examining the textual source code around the closing brace. 
> Now it's muddled: it also looks at the opening brace, and does lookups into 
> line tables.
> 
> You seem to be aiming to optimize 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 525418.
daiyousei-qz marked 9 inline comments as done.
daiyousei-qz added a comment.

- Move computeBlockEndHintRange
- Correct label length limit logic
- Correct line number computation for '{'
- Address other review comments


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/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.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Yeah, makes sense. Thank you for your insight! I missed the fact that this time 
include AST build time.

I manually injected a time region into `inlayHints`, here's part of logs 
printed for scrolling "offsetToPosition" build:

  I[23:36:43.055] <-- textDocument/hover(21)
  I[23:36:43.057] --> reply:textDocument/hover(21) 1 ms
  I[23:36:43.057] --> textDocument/clangd.fileStatus
  I[23:36:44.422] <-- textDocument/inlayHint(22)
  E[23:36:44.612] ++ Inlay hints takes 189 ms to compute
  I[23:36:44.612] --> reply:textDocument/inlayHint(22) 189 ms
  I[23:36:44.612] --> textDocument/clangd.fileStatus
  I[23:36:44.875] <-- textDocument/inlayHint(23)
  E[23:36:45.053] ++ Inlay hints takes 178 ms to compute
  I[23:36:45.054] --> reply:textDocument/inlayHint(23) 179 ms
  I[23:36:45.054] --> textDocument/clangd.fileStatus
  I[23:36:45.686] <-- textDocument/inlayHint(24)
  E[23:36:45.865] ++ Inlay hints takes 178 ms to compute
  I[23:36:45.865] --> reply:textDocument/inlayHint(24) 179 ms
  I[23:36:45.865] --> textDocument/clangd.fileStatus

And here's logs scrolling "sourceLocToPosition" + "lspLength":

  I[23:41:04.695] <-- textDocument/inlayHint(73)
  I[23:41:04.707] --> reply:textDocument/inlayHint(73) 12 ms
  I[23:41:04.707] --> textDocument/clangd.fileStatus
  I[23:41:04.793] <-- textDocument/inlayHint(74)
  I[23:41:04.807] --> reply:textDocument/inlayHint(74) 13 ms
  I[23:41:04.807] --> textDocument/clangd.fileStatus
  I[23:41:04.913] <-- textDocument/inlayHint(75)
  I[23:41:04.926] --> reply:textDocument/inlayHint(75) 12 ms
  I[23:41:04.926] --> textDocument/clangd.fileStatus
  I[23:41:05.407] <-- textDocument/inlayHint(76)
  I[23:41:05.420] --> reply:textDocument/inlayHint(76) 12 ms
  I[23:41:05.420] --> textDocument/clangd.fileStatus

As this is more than 10x slower, I'm holding my position that we shouldn't use 
`offsetToPosition` for LSP features triggered per edit/scroll. 150ms is more 
than noticeable.

If you want to repro on your side, replace computation of HintStart/HintEnd 
with the following code

  Position HintStart = offsetToPosition(MainFileBuf, RBraceOffset);
  Position HintEnd =
  offsetToPosition(MainFileBuf, RBraceOffset + HintRangeText.size());

Btw, it's always fascinating to see how modern computer system is so fast, and 
in the meantime still so "underpowered" :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D150635#4366730 , @daiyousei-qz 
wrote:

> I'm not sure but 18ms is definitely wrong here, especially for a debug build. 
> In your log, even semantic highlighting builds in 60ms, which looks 
> incorrect. I tried again with TOT clangd and noticed the following pattern.
>
> - If I scroll with vscode, inlay hints typically computes in 10~20ms
> - If I edit the source code, inlay hints computes in around 600ms

That sounds exactly like rebuilding the AST takes ~600ms and computing inlay 
hints takes <20ms.

After an edit, a request for inlay hints will wait while the AST rebuilds 
before computing inlay hints, so the request latency will show as ~620ms.

If you think you're seeing something else, please provide repro instructions 
and logs (with -log=verbose)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

I'm not sure but 18ms is definitely wrong here, especially for a debug build. 
In your log, even semantic highlighting builds in 60ms, which looks incorrect. 
I tried again with TOT clangd and noticed the following pattern.

- If I scroll with vscode, inlay hints typically computes in 10~20ms
- If I edit the source code, inlay hints computes in around 600ms

I'm not sure why that's happening. Maybe @nridge could have a better answer. 
Somehow, this patch seems introduce a significant slowdown. I'll investigate 
further to see why that's happening.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:616
+Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd());
+Position HintEnd = {HintStart.line,
+HintStart.character +

daiyousei-qz wrote:
> sammccall wrote:
> > please don't do this, call sourceLocToPosition unless you have benchmarks 
> > showing this is a bottleneck on real-world code.
> > 
> > `lspLength` and direct manipulation of line/character is unneccesarily 
> > subtle.
> > (If the performance matters, there's no need to be computing the LSP line 
> > of the lbrace at all - we never use it - this is one reason I think this 
> > function has the wrong signature)
> I argue performance does matter here. I eye-balled inlay hint compute time in 
> clangd's output window from vscode. On "./clang/lib/Sema/SemaOpenMP.cpp" 
> which is around 1MB, using `sourceLocToPosition` roughly takes around 
> 1250~1350ms. However, using two `offsetLocToPosition` usually use 
> 1400~1500ms.  I think 100ms delta is already a noticeable difference.
I'm unable to reproduce these results. After verifying EndBlock hints work in 
an editor:

```
$ bin/clangd --check=../clang/lib/Sema/SemaOpenMP.cpp
I[15:50:26.357] clangd version 17.0.0
I[15:50:26.358] Features: linux+debug-tidy
...
 I[15:50:26.358] Testing on source file 
/usr/local/google/home/sammccall/src/llvm-project/clang/lib/Sema/SemaOpenMP.cpp
...
I[15:50:26.428] Building preamble...
I[15:50:31.097] Indexing headers...
I[15:50:32.027] Built preamble of size 53468188 for file 
/usr/local/google/home/sammccall/src/llvm-project/clang/lib/Sema/SemaOpenMP.cpp 
version null in 5.60 seconds
I[15:50:32.032] Building AST...
I[15:50:38.857] Indexing AST...
I[15:50:38.975] Building inlay hints
I[15:50:38.993] Building semantic highlighting
I[15:50:39.050] Testing features at each token (may be slow in large files)
...
```

So that's 18ms for inlay hints after 5.6 seconds to parse the preamble and 6 
seconds to parse the main file. (This is a debug build, opt must be faster).

This is on a fairly powerful machine, but surely it can't be 1000x faster than 
yours - something else must be going on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, I'll try to repro those results.
100ms is significant in absolute terms, but >1s seems unacceptably slow.
I believe VSCode always sends ranges along with latency-sensitive hint 
requests, I think we currently just post-filter. If we propagate the limits 
deeper we should be able to filter much earlier and never hit these codepaths 
at all for 99% of the file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:616
+Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd());
+Position HintEnd = {HintStart.line,
+HintStart.character +

sammccall wrote:
> please don't do this, call sourceLocToPosition unless you have benchmarks 
> showing this is a bottleneck on real-world code.
> 
> `lspLength` and direct manipulation of line/character is unneccesarily subtle.
> (If the performance matters, there's no need to be computing the LSP line of 
> the lbrace at all - we never use it - this is one reason I think this 
> function has the wrong signature)
I argue performance does matter here. I eye-balled inlay hint compute time in 
clangd's output window from vscode. On "./clang/lib/Sema/SemaOpenMP.cpp" which 
is around 1MB, using `sourceLocToPosition` roughly takes around 1250~1350ms. 
However, using two `offsetLocToPosition` usually use 1400~1500ms.  I think 
100ms delta is already a noticeable difference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

> Though I'm proposing a name like "DeclBlockEnd" to make it clearer

I prefer the current name. DeclBlockEnd is both too long and contains a cryptic 
abbreviation (should be DeclarationBlockEnd). And I don't think distinguishing 
between declaration blocks and flow-control blocks is likely to be helpful.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// 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();

daiyousei-qz wrote:
> sammccall wrote:
> > This is too much arithmetic and fiddly coupling between this function and 
> > shouldHintEndDefinitionComment.
> > 
> > Among other things, it allows for confusion between unicode characters 
> > (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we 
> > do have a bug here: shouldHintEndDefinition provides SkippedChars in clang 
> > bytes, but you add it to end.character below which is in LSP characters.
> > 
> > ---
> > 
> > Let's redesign a little... 
> > 
> > We have a `}` on some line. We want to compute a sensible part of that line 
> > to attach to.
> > A suitable range may not exist, in which case we're going to omit the hint.
> > 
> > - The line consists of text we don't care about , the `}`, and then some 
> > mix of whitespace, "trivial" punctuation, and "nontrivial" chars.
> > - the range should always start at the `}`, since that's what we're really 
> > hinting
> > - to get the hint in the right place, the range should end after the 
> > trivial chars, but before any trailing whitespace
> > - if there are any nontrivial chars, there's no suitable range
> > 
> > so something like:
> > 
> > ```
> > optional findBraceTargetRange(SourceLocation CloseBrace) {
> >   auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
> >   if (File != MainFileID) return std::nullopt;
> >   StringRef RestOfLine = 
> > MainFileBuf.substr(Offset).split('\n').first.rtrim();
> >   if (!RestOfLine.consume_front("{")) return;
> >   if (StringRef::npos != Punctuation.find_first_of(" ,;")) return 
> > std::nullopt;
> >   return {offsetToPosition(MainFileBuf, Offset), 
> > offsetToPosition(MainFileBuf, Result.bytes_end() - MainFileBuf.bytes_end()};
> > }
> > ```
> > 
> > and then call from addEndDefinitionComment, the result is LSPRange already.
> Done, also moved min-line and max-length logic to this function. Btw, I think 
> we should avoid `offsetToPosition` as much as possible. It is a large 
> overhead considering inlay hints are recomputed throughout the entire file 
> for each edit. I frequently work with source code that's nearly 1MB large 
> (Yes, I don't think we should have source file this large, but it is what it 
> is).
> also moved min-line and max-length logic to this function

I'd prefer you to move them back. As specified, that function is otherwise 
strictly about examining the textual source code around the closing brace. Now 
it's muddled: it also looks at the opening brace, and does lookups into line 
tables.

You seem to be aiming to optimize an operation that you think is expensive, but 
I don't see any evidence (measurements) that this implementation is actually 
faster or that the difference matters.

> Btw, I think we should avoid offsetToPosition as much as possible

I understand the concern: computing inlay hints is quadratic. However things 
are *universally* done this way in clangd, and diverging here won't 
significantly improve performance but makes the code much harder to understand 
in context.

In practice we haven't found places where overheads that are 
length(file)*length(LSP response) are significant compared to parse times. If 
you have such examples, it would be great to gather & share profiles and 
propose a solution.
I suspect we could maintain some line lookup data structure that gets updated 
when source code updates come in over LSP, or something. But micro-optimizing 
individual codepaths isn't going to get us there: if two offsetToPosition() 
calls are bad, then one is still bad.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:575
+  // Otherwise, the hint shouldn't be shown.
+  std::optional computeBlockEndHintRange(SourceRange BraceRange,
+StringRef OptionalPunctuation) 
{

can you move this function below the related `addBlockEndHint`?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+constexpr unsigned HintMinLineLimit = 2;
+constexpr unsigned HintMaxLengthLimit = 50;
+

We actually already have a configurable inlay hint length limit.
It has the wrong name (`TypeNameLimit`) , but I think we should use it anyway 
(and maybe try to fix the name later).

(I'm not sure making this configurable was a great idea, but given that it 
exists, people will expect it to 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.

Addressed most review comments.

I'm currently using the name "BlockEnd" as suggested. Though I'm proposing a 
name like "DeclBlockEnd" to make it clearer. What do you think?




Comment at: clang-tools-extra/clangd/InlayHints.cpp:285-286
+  bool VisitEnumDecl(EnumDecl *D) {
+if (Cfg.InlayHints.EndDefinitionComments &&
+D->isThisDeclarationADefinition()) {
+  StringRef DeclPrefix;

zyounan wrote:
> nit: bail out early?
> 
No longer needed as merged into VisitTagDecl



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777
+  void addEndDefinitionCommentHint(const NamedDecl , StringRef DeclPrefix,
+   bool SkipSemicolon) {
+size_t SkippedChars = 0;

sammccall wrote:
> SkipSemicolon doesn't need to be optional, a trailing semicolon never adds 
> any ambiguity about what the hint is for
Yes, no ambiguity. But the following hint looks weird to me:
```
void foo() {
}; // foo
```

Since this isn't that complicated to filter them out, I'd prefer making it more 
precise.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// 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();

sammccall wrote:
> This is too much arithmetic and fiddly coupling between this function and 
> shouldHintEndDefinitionComment.
> 
> Among other things, it allows for confusion between unicode characters 
> (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we do 
> have a bug here: shouldHintEndDefinition provides SkippedChars in clang 
> bytes, but you add it to end.character below which is in LSP characters.
> 
> ---
> 
> Let's redesign a little... 
> 
> We have a `}` on some line. We want to compute a sensible part of that line 
> to attach to.
> A suitable range may not exist, in which case we're going to omit the hint.
> 
> - The line consists of text we don't care about , the `}`, and then some mix 
> of whitespace, "trivial" punctuation, and "nontrivial" chars.
> - the range should always start at the `}`, since that's what we're really 
> hinting
> - to get the hint in the right place, the range should end after the trivial 
> chars, but before any trailing whitespace
> - if there are any nontrivial chars, there's no suitable range
> 
> so something like:
> 
> ```
> optional findBraceTargetRange(SourceLocation CloseBrace) {
>   auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
>   if (File != MainFileID) return std::nullopt;
>   StringRef RestOfLine = MainFileBuf.substr(Offset).split('\n').first.rtrim();
>   if (!RestOfLine.consume_front("{")) return;
>   if (StringRef::npos != Punctuation.find_first_of(" ,;")) return 
> std::nullopt;
>   return {offsetToPosition(MainFileBuf, Offset), 
> offsetToPosition(MainFileBuf, Result.bytes_end() - MainFileBuf.bytes_end()};
> }
> ```
> 
> and then call from addEndDefinitionComment, the result is LSPRange already.
Done, also moved min-line and max-length logic to this function. Btw, I think 
we should avoid `offsetToPosition` as much as possible. It is a large overhead 
considering inlay hints are recomputed throughout the entire file for each 
edit. I frequently work with source code that's nearly 1MB large (Yes, I don't 
think we should have source file this large, but it is what it is).



Comment at: clang-tools-extra/clangd/InlayHints.cpp:797
+  // differently.
+  assert(Label.empty());
+  Label += printName(AST, D);

zyounan wrote:
> nit: `assert(Label.empty() && "Label should be empty with FunctionDecl")`. 
> might be helpful for debugging.
No longer needed as this assert is removed.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:671
+
+  void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+llvm::StringRef Prefix, llvm::StringRef Label,

sammccall wrote:
> I don't really like this signature change.
> 
> I understand the goal to avoid duplicating the range computation but it seems 
> unlikely to be critical.
> Also, the caller could get the line numbers of the `{}` from the 
> SourceManager and compare those, which should be cheaper than computing the 
> range, so we wouldn't be duplicating all of the work.
As per another comment below, this change is kept.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:760
+// Note this range doesn't include the trailing ';' in type definitions.
+SourceRange R = D.getSourceRange();
+auto LSPRange = getHintRange(R);

sammccall wrote:
> I believe it makes more sense to target the `}` rather than the whole 
> declaration here - we're really talking about what the `}` closes, rather 
> than 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 524183.
daiyousei-qz marked 19 inline comments as done.
daiyousei-qz added a comment.

Address review comments


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/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.BlockEnd = false;
   return C;
 }
 
@@ -122,6 +123,15 @@
   assertHints(InlayHintKind::Designator, AnnotatedSource, Expected...);
 }
 
+template 
+void assertBlockEndHints(llvm::StringRef AnnotatedSource,
+ ExpectedHints... Expected) {
+  Config Cfg;
+  Cfg.InlayHints.BlockEnd = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  assertHints(InlayHintKind::BlockEnd, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1560,194 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(BlockEndHints, Functions) {
+  assertBlockEndHints(R"cpp(
+int foo() {
+  return 41;
+$foo[[}]]
+
+template 
+int bar() { 
+  // No hint for lambda for now
+  auto f = []() { 
+return X; 
+  };
+  return f(); 
+$bar[[}]]
+
+// No hint because this isn't a definition
+int buz();
+
+struct S{};
+bool operator==(S, S) {
+  return true;
+$opEqual[[}]]
+  )cpp",
+  ExpectedHint{" // foo", "foo"},
+  ExpectedHint{" // bar", "bar"},
+  ExpectedHint{" // operator==", "opEqual"});
+}
+
+TEST(BlockEndHints, Methods) {
+  assertBlockEndHints(R"cpp(
+struct Test {
+  // No hint because there's no function body
+  Test() = default;
+  
+  ~Test() {
+  $dtor[[}]]
+
+  void method1() {
+  $method1[[}]]
+
+  // No hint because this isn't a definition
+  void method2();
+
+  template 
+  void method3() {
+  $method3[[}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+
+  Test operator+(int) const {
+return *this;
+  $opIdentity[[}]]
+
+  operator bool() const {
+return true;
+  $opBool[[}]]
+
+  // No hint because there's no function body
+  operator int() const = delete;
+} x;
+
+void Test::method2() {
+$method2[[}]]
+
+template 
+void Test::method4() {
+$method4[[}]]
+  )cpp",
+  ExpectedHint{" // ~Test", "dtor"},
+  ExpectedHint{" // method1", "method1"},
+  ExpectedHint{" // method3", "method3"},
+  ExpectedHint{" // operator+", "opIdentity"},
+  ExpectedHint{" // operator bool", "opBool"},
+  ExpectedHint{" // Test::method2", "method2"},
+  ExpectedHint{" // Test::method4", "method4"});
+}
+
+TEST(BlockEndHints, Namespaces) {
+  assertBlockEndHints(
+  R"cpp(
+namespace {
+  void foo();
+$anon[[}]]
+
+namespace ns {
+  void bar();
+$ns[[}]]
+  )cpp",
+  ExpectedHint{" // namespace", "anon"},
+  ExpectedHint{" // namespace ns", "ns"});
+}
+
+TEST(BlockEndHints, Types) {
+  assertBlockEndHints(
+  R"cpp(
+struct S {
+$S[[};]]
+
+class C {
+$C[[};]]
+
+union U {
+$U[[};]]
+
+enum E1 {
+$E1[[};]]
+
+enum class E2 {
+$E2[[};]]
+  )cpp",
+  ExpectedHint{" // struct S", "S"}, ExpectedHint{" // class C", "C"},
+  ExpectedHint{" // union U", "U"}, ExpectedHint{" // enum E1", "E1"},
+  ExpectedHint{" // enum class E2", "E2"});
+}
+
+TEST(BlockEndHints, TrailingSemicolon) {
+  assertBlockEndHints(R"cpp(
+// The hint is placed after the trailing ';'
+struct S1 {
+$S1[[}  ;]]   
+
+// The hint is always placed in the same line with the closing '}'.
+// So in this case where ';' is missing, it is attached to '}'.
+struct S2 {
+$S2[[}]]
+
+;
+
+// No hint because only one trailing ';' is allowed
+struct S3 {
+};;
+
+// No hint because trailing ';' is only allowed for class/struct/union/enum
+void foo() {
+};
+
+// Rare case, but yes we'll have a hint here.
+struct {
+  

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D150635#4348702 , @daiyousei-qz 
wrote:

> Regarding to **Naming**, I agree that `EndDefinitionComment` sounds so 
> tedious but I couldn't come up with a better name. However, I think 
> `EndBlock` also feel a little strange to me as a public name. If I come 
> across a setting key in `InlayHints` section named `EndBlock`, honestly I 
> would be confused by what it would do ("end" could be a verb, and "block" 
> usually make me to think of the local scope block). What about `AfterBlock` 
> or `AfterDefinition`?

Agreed that "End" looks too much like a verb here. "After" isn't quite right, 
again because we're trying to describe the thing we're hinting, not where the 
hint is position.
I prefer "Block" over "Definition" exactly because this sort of hint should 
also be added to local scope blocks (e.g. `if` bodies) later.

Does `BlockEnd` address your concern? ("Block" can be a verb but I think 
there's no risk of confusion here)

> Regarding to **Configuration**, I suppose the minimum line number of 
> definition is a very natural parameter for this feature. I doubt we could 
> come up with some good heuristic to conditionally show this hint. Considering 
> how straightforward this parameter its implementation is, I'd prefer to 
> having this around if possible. But I'll let you to decide.

Right: It's tempting to make it an option, because we don't know what the value 
should be, and an option is easy to implement.

But these are not **good** reasons to make it an option:

- an option doesn't substantially change the importance of picking a good 
default heuristic, because 95+% of people will use it anyway. If we can't find 
a good default, we shouldn't implement the feature.
- the vast majority of the cost of an option is maintenance and user-facing 
complexity, rather than implementation




Comment at: clang-tools-extra/clangd/InlayHints.cpp:278
+if (Cfg.InlayHints.EndDefinitionComments &&
+D->isThisDeclarationADefinition()) {
+  addEndDefinitionCommentHint(*D, "", false);

I think this is a wrong/incomplete check, `e.g. Foo(const Foo&) = default` is a 
definition but doesn't have a brace range.

Maybe it's less redundant just to try to get the brace range and then check if 
it's valid.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777
+  void addEndDefinitionCommentHint(const NamedDecl , StringRef DeclPrefix,
+   bool SkipSemicolon) {
+size_t SkippedChars = 0;

SkipSemicolon doesn't need to be optional, a trailing semicolon never adds any 
ambiguity about what the hint is for



Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+// 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();

This is too much arithmetic and fiddly coupling between this function and 
shouldHintEndDefinitionComment.

Among other things, it allows for confusion between unicode characters 
(UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we do 
have a bug here: shouldHintEndDefinition provides SkippedChars in clang bytes, 
but you add it to end.character below which is in LSP characters.

---

Let's redesign a little... 

We have a `}` on some line. We want to compute a sensible part of that line to 
attach to.
A suitable range may not exist, in which case we're going to omit the hint.

- The line consists of text we don't care about , the `}`, and then some mix of 
whitespace, "trivial" punctuation, and "nontrivial" chars.
- the range should always start at the `}`, since that's what we're really 
hinting
- to get the hint in the right place, the range should end after the trivial 
chars, but before any trailing whitespace
- if there are any nontrivial chars, there's no suitable range

so something like:

```
optional findBraceTargetRange(SourceLocation CloseBrace) {
  auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
  if (File != MainFileID) return std::nullopt;
  StringRef RestOfLine = MainFileBuf.substr(Offset).split('\n').first.rtrim();
  if (!RestOfLine.consume_front("{")) return;
  if (StringRef::npos != Punctuation.find_first_of(" ,;")) return std::nullopt;
  return {offsetToPosition(MainFileBuf, Offset), offsetToPosition(MainFileBuf, 
Result.bytes_end() - MainFileBuf.bytes_end()};
}
```

and then call from addEndDefinitionComment, the result is LSPRange already.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:292
+
+  bool VisitRecordDecl(RecordDecl *D) {
+if (Cfg.InlayHints.EndDefinitionComments &&

sammccall wrote:
> Not sure why if need to handle this + EnumDecl, can we just handle 
> VisitTagDecl to cover both?
This is marked done but isn't done.
I'd suggest something like

```

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thank you for the update. A few questions:

- Can you add a test with macro expansion? Would we get two inlay hints right 
after the spelling location? e.g.,

  #define Decl_Record \
struct Widget {  \
  [very long definition that reach `EndDefinitionCommentMinLines`]. \
  struct SubWidget { \
[another chunk of members.] \
  // not ending with a bracket here!
  
  //  but appears here:
  //  v   v
  Decl_Record } // struct SubWidget ; } // struct Widget (Hmm...) ;



- I was thinking if we should add some test cases for C. (I have learned that 
it is allowed to write a declaration in an expression while C++ doesn't.) For 
example,

  int main(void) {
_Alignof(struct S {
  [long definition]
} // Is it suitable to hint here? What do you think?
);
  }




Comment at: clang-tools-extra/clangd/InlayHints.cpp:285-286
+  bool VisitEnumDecl(EnumDecl *D) {
+if (Cfg.InlayHints.EndDefinitionComments &&
+D->isThisDeclarationADefinition()) {
+  StringRef DeclPrefix;

nit: bail out early?




Comment at: clang-tools-extra/clangd/InlayHints.cpp:797
+  // differently.
+  assert(Label.empty());
+  Label += printName(AST, D);

nit: `assert(Label.empty() && "Label should be empty with FunctionDecl")`. 
might be helpful for debugging.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1716
+
+// No hint becaus trailing ';' is only allowed for class/struct/union/enum
+void foo() {

typo: `becaus` -> `because`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

For a quick peek to the hint:

F27483170: image.png 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

@zyounan np! Feel free to leave your review comments!

@sammccall Thank you for you insightful and detailed review! I addressed many 
review comments in the updated patch and left things I'm not very sure.

Regarding to **Naming**, I agree that `EndDefinitionComment` sounds so tedious 
but I couldn't come up with a better name. However, I think `EndBlock` also 
feel a little strange to me as a public name. If I come across a setting key in 
`InlayHints` section named `EndBlock`, honestly I would be confused by what it 
would do ("end" could be a verb, and "block" usually make me to think of the 
local scope block). What about `AfterBlock` or `AfterDefinition`?

Regarding to **Configuration**, I suppose the line of definition is a very 
natural parameter for this feature. I doubt we could come up with some good 
heuristic to conditional show this hint. Considering how straightforward this 
parameter its implementation is, I'd prefer to having this around if possible. 
But I'll let you to decide.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:763
+if (!LSPRange ||
+static_cast(LSPRange->end.line - LSPRange->start.line) + 1 <
+Cfg.InlayHints.EndDefinitionCommentMinLines)

sammccall wrote:
> I'm not sure we're using the right range here. Consider:
> 
> ```
> const some::long* function
>many params, therefore spanning, lines) {}
> ```
> 
> I don't think hinting `}` here is useful: the relevant distance is from `{` 
> rather than the start of the declaration.
I agree. What do you think is a good way get the range of the definition block? 
What I can think of is to walking backwards the expanded token buffer from the 
ending '}' and searching for the balanced '}'.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:771
+if (const auto *FD = dyn_cast_or_null()) {
+  Label = printName(AST, D);
+} else {

sammccall wrote:
> why is printName OK here and not below?
I'm using printName here to print names for operator overload. Do you have any 
advice?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777-781
+  else if (isa(D)) {
+Label += "enum ";
+if (cast(D).isScopedUsingClassTag()) {
+  Label += "class ";
+}

sammccall wrote:
> zyounan wrote:
> > Perhaps duplicate the logic from [[ 
> > https://clang.llvm.org/doxygen/DeclPrinter_8cpp_source.html#l00529 | 
> > DeclPrinter::VisitEnumDecl ]]? For `enum struct`, printing `enum` only 
> > might confuse users.
> > 
> I'm torn here: "enum class Foo" is wordy so "enum Foo" is tempting to me. 
> Don't have a strong opinion.
> 
> regardless, printing the tag for class but not struct is definitely wrong :-)
Fixed `enum struct`. Honestly, just learnt there's such a thing, lol.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1566
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(

sammccall wrote:
> can you add testcases where:
>  - the function name is an operator
>  - the function is templated
Added template function. I don't quite understand what you mean by "the 
function name is an operator". I'm assuming you meant operator overload so 
closing this one as covered below.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1576
+  )cpp",
+   0, ExpectedHint{" /* foo */ ", "foo"},
+   ExpectedHint{" /* bar */ ", "bar"});

sammccall wrote:
> we're not adding the kind here as in "function foo".
> 
> Maybe that's OK, "function" is long, doesn't appear in the C++ syntax, and is 
> mostly obvious.
> But in that case I'd like to have some hint, maybe `/* foo() */`? (Not the 
> actual param list of course, the parens would always be empty)
> 
> If you don't think we need any hint, then I'd question whether we need 
> "class" etc for class hints!
I think just `// foo` is good enough. Having no "qualifier" could be a 
privilege for functions since they are usually the majority of the source code. 
When reading through the source code, people will get that if no prefix is 
attached, then it's a function. (OFC they'll also see `// namespace` and .etc, 
but rare)

Honestly, IMO `// foo()` is worse than just the name. It could mislead me to 
think that this may be a complete signature while it is definitely not.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1580
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(

sammccall wrote:
> can you add a test with a lambda?
> I think the hint should probably just be `lambda` or `(lambda)`, this 
> probably needs special handling.
> 
> It definitely shouldn't be `operator()` or `(lambda at somefile.cc:47)`.
I would argue we don't need hint for a lambda. This hint is only added to 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

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 
+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 
+$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 
+  $method3[[void method3() {}]]
+
+  // No hint because this isn't a definition
+  template 
+  void method4();
+} x;
+
+$method2[[void Test::method2() {}]]
+
+template 
+$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.
+ 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:292
+
+  bool VisitRecordDecl(RecordDecl *D) {
+if (Cfg.InlayHints.EndDefinitionComments &&

Not sure why if need to handle this + EnumDecl, can we just handle VisitTagDecl 
to cover both?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:570
+  // To avoid clash with manual annotation from users, we only show this hint 
if
+  // there's no character after '}' except for whitespace and ';'.
+  // Note this also allows hint to be shown for cases like:

this part of the comment is just echoing the code (too much detail). Maybe 
something like

```
Only add end-block hints if the rest of the line is trivial.

In particular, we avoid hinting:
 - if a user comment might provide the same information
 - when it might not be clear what construct the hint is for
```

(The latter is mostly important if we switch to line-comments: if there's `}}` 
on a line we don't want to hint the inner one!)



Comment at: clang-tools-extra/clangd/InlayHints.cpp:571
+  // there's no character after '}' except for whitespace and ';'.
+  // Note this also allows hint to be shown for cases like:
+  //   struct S {

Not only is this rare, but it's not clear hinting is particularly undesirable 
here - I'd just drop this comment.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:583
+StringRef SourceSuffix =
+MainFileBuf.substr(Decomposed.second + 1).ltrim("; \v\f\r");
+return SourceSuffix.empty() || SourceSuffix.starts_with("\n");

The +1 is suspicious. `if (!Suffix.consume_front("{")) return false;` would be 
clearer and hold up better in the presence of macros.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:671
+
+  void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+llvm::StringRef Prefix, llvm::StringRef Label,

I don't really like this signature change.

I understand the goal to avoid duplicating the range computation but it seems 
unlikely to be critical.
Also, the caller could get the line numbers of the `{}` from the SourceManager 
and compare those, which should be cheaper than computing the range, so we 
wouldn't be duplicating all of the work.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:760
+// Note this range doesn't include the trailing ';' in type definitions.
+SourceRange R = D.getSourceRange();
+auto LSPRange = getHintRange(R);

I believe it makes more sense to target the `}` rather than the whole 
declaration here - we're really talking about what the `}` closes, rather than 
what the declaration itself is.

This is sort of conceptual - at the protocol level the target range doesn't 
really make a difference.

There is one practical difference though: this affects whether we allow hinting 
in scenarios where (part of) the declaration is expanded from macros.

It's also more consistent with the other hints, which (generally) use fairly 
small ranges. So if we use the range for something else in future it's likely 
to play better.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:763
+if (!LSPRange ||
+static_cast(LSPRange->end.line - LSPRange->start.line) + 1 <
+Cfg.InlayHints.EndDefinitionCommentMinLines)

I'm not sure we're using the right range here. Consider:

```
const some::long* function
   many params, therefore spanning, lines) {}
```

I don't think hinting `}` here is useful: the relevant distance is from `{` 
rather than the start of the declaration.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:767
+
+/// TODO: We could use InlayHintLabelPart to provide language features on
+/// hints.

This is kind of true for all inlay hints (not just end-definition), and we 
haven't really decided to do so, so I don't think we need this comment.



Comment at: clang-tools-extra/clangd/InlayHints.cpp:771
+if (const auto *FD = dyn_cast_or_null()) {
+  Label = printName(AST, D);
+} else {

why is printName OK here and not below?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:772
+  Label = printName(AST, D);
+} else {
+  // We handle type and namespace decls together.

zyounan wrote:
> Question: Should we restrict the type here? What will happen if `D` was 
> something unexpected like a `TypedefDecl`? Would it fall into the logic that 
> makes `Label` result to ``?
Yeah, this case-enumeration feels a bit fuzzy to me, like we had more 
structured information earlier and are now trying to recover it.

I'd like the signature of this function to be a bit more concrete, e.g. taking 
parameters:

 - `SourceRange Braces`
 - `StringRef Kind` (e.g. "class")
 

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this! Some high-level comments here, will review 
implementation next:

**There seems to be a lot of "decoration" on the hint text.**

If declaring a class "Foo" then we add ` /* class ` before and ` */ ` after 
Foo, for a total of 14 characters apart from the name itself. Even if we're 
mostly adding these hints to mostly-empty lines, I think we should try to be a 
bit more economical to reduce breaking up the flow of the code.

We could reduce this by some combination of:

- not using comment syntax, e.g. ` class Foo` or `(class Foo)`. We've used 
pseudo-C++ syntax for other hints, but largely because it was convenient and 
terse.
- using `// ` rather than `/*  */` comment syntax where possible, e.g. `// 
class Foo`
- dropping the kind, e.g. `/* Foo */`
- dropping the name, e.g. `/* class */`

I think my favorite of these is probably `// class Foo` - this is 5 characters 
shorter than the current version, provides all the information, uses pseudo-C++ 
syntax, is consistent with clang-format's namespace comments, and would be 
suitable for a "insert closing comment" code action in future.

This implies putting the hints after the semicolon (I think it's fine to do 
this purely textually by examining characters), and choosing some behavior when 
there's a trailing comment or code (my suggestion would be just to drop the 
hint in this case).

WDYT?

**Naming**: `EndDefinitionComment` is a bit of a mouthful, I'd like something 
simpler.

"Comment" is the form of the hint, not the thing being hinted, so it seems 
superfluous. ("Designator" is similar, but I couldn't think of a good 
alternative there).
"Definition" is OK but a little vague - many things have definitions, so it's 
not clear what we'll hint. I think "Block" is clearer. It could describe e.g. 
the end of a long `while` loop, but I think that's actually OK - we could add 
that one day, and I think it would make sense to have it controlled by the same 
setting.

So I think my favorite name would be `EndBlock` - most critically in the 
configuration, but also consistently in the code.
(Renaming things is a lot of churn, feel free to defer changes until we've got 
a solid agreement on a name!)

**Configurability**: Does the min-lines really need a config knob now?

Fairly few people change the defaults, so:

- we'll need to find the best default in any case, and
- to justify adding an option it should be *very* useful to the people that use 
it

In particular, "the default is arbitrary" is not itself a strong reason to make 
it configurable.
The downsides to configuration are extra testing/plumbing, and we also lock 
ourselves into "number of lines" being the criterion - maybe we want a 
different heuristic in future and this will interfere.

If we ship without it, we'll get clear feedback on whether the default is good 
and if customization is needed, and it's easy to add an option later.
Unless I'm missing something, I'd prefer this to be a constant in the code for 
now.

I think the default of 2 you've got here is worth a try, maybe after some 
experience a higher number will be better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Sorry for chiming in. Left a few nit comments and I hope you don't mind. :)




Comment at: clang-tools-extra/clangd/InlayHints.cpp:772
+  Label = printName(AST, D);
+} else {
+  // We handle type and namespace decls together.

Question: Should we restrict the type here? What will happen if `D` was 
something unexpected like a `TypedefDecl`? Would it fall into the logic that 
makes `Label` result to ``?



Comment at: clang-tools-extra/clangd/InlayHints.cpp:777-781
+  else if (isa(D)) {
+Label += "enum ";
+if (cast(D).isScopedUsingClassTag()) {
+  Label += "class ";
+}

Perhaps duplicate the logic from [[ 
https://clang.llvm.org/doxygen/DeclPrinter_8cpp_source.html#l00529 | 
DeclPrinter::VisitEnumDecl ]]? For `enum struct`, printing `enum` only might 
confuse users.




Comment at: clang-tools-extra/clangd/InlayHints.cpp:783-788
+if (RecordD->isStruct())
+  Label += "struct ";
+else if (RecordD->isClass())
+  Label += "class ";
+else if (RecordD->isUnion())
+  Label += "union ";

nit: How about using [[ 
https://clang.llvm.org/doxygen/Decl_8h_source.html#l03645 | 
tagDecl::getKindName() ]]?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

Requesting for some advice




Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1666
+
+$anon[[struct {
+  int x;

This is unwanted behavior from my understanding. Do you guys have any insight 
how we could fix this? What I can think of is tracking if we are currently 
inside a variable declaration and turning off the hint. However, this would 
have some side effects on
```
auto f = [] {
   struct S {};
};
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150635/new/

https://reviews.llvm.org/D150635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 522434.
daiyousei-qz added a comment.

minor naming fix2 (last fix breaks builds)


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 
+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,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// 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:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )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(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /* struct S3 */ ", "S3"});
+}
+
+TEST(EndDefinitionHints, TrailingText) {
+  

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz updated this revision to Diff 522432.
daiyousei-qz edited the summary of this revision.
daiyousei-qz added a comment.

minor naming fix


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 
+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,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// 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:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )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(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /* struct S3 */ ", "S3"});
+}
+

[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
daiyousei-qz requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

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 
+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::EndDefinitionComments, AnnotatedSource,
+  Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
 void foo(int param);
@@ -1550,6 +1562,165 @@
   ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
   ExpectedHint{"c: ", "param3"});
 }
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
+$foo[[int foo() {
+  return 41;
+}]]
+$bar[[int bar() {}]]
+
+// 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:
+  $ctor[[Test() = default]];
+  $dtor[[~Test() {
+  }]]
+
+  $method[[void method() {}]]
+
+  // No hint because this isn't a definition
+  void method2();
+} x;
+  )cpp",
+   0, ExpectedHint{" /* Test */ ", "ctor"},
+   ExpectedHint{" /* ~Test */ ", "dtor"},
+   ExpectedHint{" /* method */ ", "method"});
+}
+
+TEST(EndDefinitionHints, OverloadedOperators) {
+  assertEndDefinitionHints(R"cpp(
+struct S {
+  $opAdd[[S operator+(int) const {
+return *this;
+  }]]
+
+  $opBool[[operator bool() const {
+return true;
+  }]]
+
+  $opInt[[operator int() const = delete]];
+
+  // No hint because this isn't a definition
+  operator float() const;
+} x;
+  )cpp",
+   0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+   ExpectedHint{" /* operator bool */ ", "opBool"},
+   ExpectedHint{" /* operator int */ ", "opInt"});
+}
+
+TEST(EndDefinitionHints, Namespaces) {
+  assertEndDefinitionHints(
+  R"cpp(
+$anon[[namespace {
+}]]
+
+$ns[[namespace ns {
+  void foo();
+}]]
+  )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(
+struct {
+  int x;
+} s;
+
+$anon[[struct {
+  int x;
+}]]
+
+s2;
+  )cpp",
+  0, ExpectedHint{" /* struct  */ ", "anon"});
+}
+
+TEST(EndDefinitionHints, TrailingSemicolon) {
+  assertEndDefinitionHints(R"cpp(
+$S1[[struct S1 {
+}]];
+
+$S2[[struct S2 {
+}]]
+
+;
+
+$S3[[struct S3 {
+}]] ;; ;;
+  )cpp",
+   0, ExpectedHint{" /* struct S1 */ ", "S1"},
+   ExpectedHint{" /* struct S2 */ ", "S2"},
+   ExpectedHint{" /*