sammccall added a comment.
> Though I'm proposing a name like "DeclBlockEnd" to make it clearer
I prefer the current name. DeclBlockEnd is both too long and contains a cryptic abbreviation (should be DeclarationBlockEnd). And I don't think distinguishing between declaration blocks and flow-control blocks is likely to be helpful.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:783
+ // Note this range doesn't include the trailing ';' in type definitions.
+ // So we have to add SkippedChars to the end character.
+ SourceRange R = D.getSourceRange();
----------------
daiyousei-qz wrote:
> sammccall wrote:
> > This is too much arithmetic and fiddly coupling between this function and shouldHintEndDefinitionComment.
> >
> > Among other things, it allows for confusion between unicode characters (UTF-32), clang bytes (UTF-8), and LSP characters (usually UTF-16). And we do have a bug here: shouldHintEndDefinition provides SkippedChars in clang bytes, but you add it to end.character below which is in LSP characters.
> >
> > ---
> >
> > Let's redesign a little...
> >
> > We have a `}` on some line. We want to compute a sensible part of that line to attach to.
> > A suitable range may not exist, in which case we're going to omit the hint.
> >
> > - The line consists of text we don't care about , the `}`, and then some mix of whitespace, "trivial" punctuation, and "nontrivial" chars.
> > - the range should always start at the `}`, since that's what we're really hinting
> > - to get the hint in the right place, the range should end after the trivial chars, but before any trailing whitespace
> > - if there are any nontrivial chars, there's no suitable range
> >
> > so something like:
> >
> > ```
> > optional<Range> findBraceTargetRange(SourceLocation CloseBrace) {
> > auto [File, Offset] = SM.getDecomposedLoc(SM.getFileLoc(CloseBrace));
> > if (File != MainFileID) return std::nullopt;
> > StringRef RestOfLine = MainFileBuf.substr(Offset).split('\n').first.rtrim();
> > if (!RestOfLine.consume_front("{")) return;
> > if (StringRef::npos != Punctuation.find_first_of(" ,;")) return std::nullopt;
> > return {offsetToPosition(MainFileBuf, Offset), offsetToPosition(MainFileBuf, Result.bytes_end() - MainFileBuf.bytes_end()};
> > }
> > ```
> >
> > and then call from addEndDefinitionComment, the result is LSPRange already.
> Done, also moved min-line and max-length logic to this function. Btw, I think we should avoid `offsetToPosition` as much as possible. It is a large overhead considering inlay hints are recomputed throughout the entire file for each edit. I frequently work with source code that's nearly 1MB large (Yes, I don't think we should have source file this large, but it is what it is).
> also moved min-line and max-length logic to this function
I'd prefer you to move them back. As specified, that function is otherwise strictly about examining the textual source code around the closing brace. Now it's muddled: it also looks at the opening brace, and does lookups into line tables.
You seem to be aiming to optimize an operation that you think is expensive, but I don't see any evidence (measurements) that this implementation is actually faster or that the difference matters.
> Btw, I think we should avoid offsetToPosition as much as possible
I understand the concern: computing inlay hints is quadratic. However things are *universally* done this way in clangd, and diverging here won't significantly improve performance but makes the code much harder to understand in context.
In practice we haven't found places where overheads that are length(file)*length(LSP response) are significant compared to parse times. If you have such examples, it would be great to gather & share profiles and propose a solution.
I suspect we could maintain some line lookup data structure that gets updated when source code updates come in over LSP, or something. But micro-optimizing individual codepaths isn't going to get us there: if two offsetToPosition() calls are bad, then one is still bad.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:575
+ // Otherwise, the hint shouldn't be shown.
+ std::optional<Range> computeBlockEndHintRange(SourceRange BraceRange,
+ StringRef OptionalPunctuation) {
----------------
can you move this function below the related `addBlockEndHint`?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+ constexpr unsigned HintMinLineLimit = 2;
+ constexpr unsigned HintMaxLengthLimit = 50;
+
----------------
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)
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:590
+
+ StringRef TrimedTrailingText = RestOfLine.drop_front().trim();
+ if (!TrimedTrailingText.empty() &&
----------------
nit: Trimed => Trimmed, or just shorten the name
scope of var can be reduced here:
```
if (StringRef TrailingText = ...; !TrailingText.empty() && ...)
return std::nullopt
```
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:597
+ auto BlockBeginLine =
+ SM.getSpellingLineNumber(BraceRange.getBegin(), &Invalid);
+ if (Invalid)
----------------
this should be getFileLoc(BraceRange.getBegin()), I think?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:598
+ SM.getSpellingLineNumber(BraceRange.getBegin(), &Invalid);
+ if (Invalid)
+ return std::nullopt;
----------------
we can drop all the invalid checks. These are ~never going to fire, and aren't the best condition to check when they do fire.
For RBrace, we already know it's in the main file (can't be invalid)
For LBrace, something weird like token-pasting or an inline `#include` needs to happen for this to be invalid. But if you want to check this, a better test would be to decompose LBrace like you do RBrace and check that the FileID is right. Because if you somehow end up in a different file, the line number will be "valid" but the comparison is not meaningful.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:605
+
+ if (BlockBeginLine + HintMinLineLimit - 1 > RBraceLine)
+ return std::nullopt;
----------------
motivating comment would be useful here:
// Don't show hints on trivial blocks like `class X {};`
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:616
+ Position HintStart = sourceLocToPosition(SM, BraceRange.getEnd());
+ Position HintEnd = {HintStart.line,
+ HintStart.character +
----------------
please don't do this, call sourceLocToPosition unless you have benchmarks showing this is a bottleneck on real-world code.
`lspLength` and direct manipulation of line/character is unneccesarily subtle.
(If the performance matters, there's no need to be computing the LSP line of the lbrace at all - we never use it - this is one reason I think this function has the wrong signature)
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:671
+
+ void addInlayHint(Range LSPRange, HintSide Side, InlayHintKind Kind,
+ llvm::StringRef Prefix, llvm::StringRef Label,
----------------
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.
================
Comment at: clang-tools-extra/clangd/Protocol.h:1656
+ /// } ^
+ /// Uses comment-like syntax like "/* func */".
+ /// This is a clangd extension.
----------------
nit: update the comment syntax
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150635/new/
https://reviews.llvm.org/D150635