[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<basic_string<char, char_traits<char>, allocator<char>, allocator<basic_string<...>>` 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 <typename T>
struct S {
  void foo();
};

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

is the hint at the indicated location `S::foo()`, or `S<T>::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