[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<uint32_t>(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<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.


================
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<FunctionDecl>(&D)) {
+      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 `<anonymous>`?
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")
 - `DeclarationName Name` (we can overload/change the signature of getSimpleName)

This is also more similar to the other functions in this family.



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:774
+      // We handle type and namespace decls together.
+      // Note we don't use printName here for formatting issues.
+      if (isa<NamespaceDecl>(D))
----------------
if you're not going to say what the issues are then there's not much point in the comment!


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:777-781
+      else if (isa<EnumDecl>(D)) {
+        Label += "enum ";
+        if (cast<EnumDecl>(D).isScopedUsingClassTag()) {
+          Label += "class ";
+        }
----------------
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 :-)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1566
+
+TEST(EndDefinitionHints, Functions) {
+  assertEndDefinitionHints(R"cpp(
----------------
can you add testcases where:
 - the function name is an operator
 - the function is templated


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1576
+  )cpp",
+                           0, ExpectedHint{" /* foo */ ", "foo"},
+                           ExpectedHint{" /* bar */ ", "bar"});
----------------
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!


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1580
+
+TEST(EndDefinitionHints, Methods) {
+  assertEndDefinitionHints(R"cpp(
----------------
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)`.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1587
+      }]]
+
+      $method[[void method() {}]]
----------------
can you add a template method?
(These are weird because they're functiondecls rather than methoddecls, I think)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1592
+      void method2();
+    } x;
+  )cpp",
----------------
can you add an out-of-line definition for method2?

In particular we need to decide whether the hint will be `Test::method2` or `method2`.
(This also serves as a test for definitions of functions via qualified names - they should work the same way)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1617
+                           0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+                           ExpectedHint{" /* operator bool */ ", "opBool"},
+                           ExpectedHint{" /* operator int */ ", "opInt"});
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1631
+  )cpp",
+      0, ExpectedHint{" /* namespace <anonymous> */ ", "anon"},
+      ExpectedHint{" /* namespace ns */ ", "ns"});
----------------
I think this should just be "namespace" for consistency with clang-format

This probably goes for all unnamed decls, it also matches the C++ syntax


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1666
+
+    $anon[[struct {
+      int x;
----------------
daiyousei-qz wrote:
> 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 {};
> };
> ```
This behavior looks totally fine to me, I certainly wouldn't go out of my way to change it.

if s2 were on the same line, we'd suppress the class hint, and I think that's also fine (esp if we switch to line-comment syntax)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150635