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<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.
> 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 <typename T>
> struct S {
> void foo();
> };
>
> template <typename T>
> void S<T>::foo() {
> } // S<T>::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<int>` or so)
================
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:
> daiyousei-qz wrote:
> > 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.
> Please revert this unless you can show a significant performance difference
> in a real-world setting.
OK, I'm convinced we need this. Not for performance, but because it's not
natural to obtain a closed token range in this case where we're manipulating
character by character.
================
Comment at: clang-tools-extra/clangd/Protocol.cpp:1438
case InlayHintKind::Designator: // This is an extension, don't serialize.
+ case InlayHintKind::BlockEnd: // This is an extension, don't
+ // serialize.
----------------
nit: move the comment to the return statement rather than repeating it
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
