[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<uint32_t>(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<type, name>* 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<FunctionDecl>(&D)) {
+      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<EnumDecl>(D)) {
+        Label += "enum ";
+        if (cast<EnumDecl>(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 long definitions that may introduce a symbol. It is helpful to highlight the boundaries of different declarations. Because of the implementation assumes the hint is added to a `NamedDecl`, lambda should be safe.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1617
+                           0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+                           ExpectedHint{" /* operator bool */ ", "opBool"},
+                           ExpectedHint{" /* operator int */ ", "opInt"});
----------------
sammccall wrote:
> I'm not certain this is a good idea - we're printing types, these can be very long, have various levels of sugar.
> 
> Elsewhere where we print types we need to apply a length cutoff, we could try to do that here or just bail out for now on certain types of names.
I agree we should avoid very long hints being displayed. Instead of a length cutoff on type, however, I suppose a limit on the entire hint would be much simpler to implement. Do you think that's okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635