[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