[PATCH] D150635: [clangd] Implement end-definition-comment inlay hints
Qingyuan Zheng via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 22 00:59:18 PDT 2023
daiyousei-qz marked an inline comment as done.
daiyousei-qz added a comment.
Addressed most review comments.
I'm currently using the name "BlockEnd" as suggested. Though I'm proposing a name like "DeclBlockEnd" to make it clearer. What do you think?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:285-286
+ bool VisitEnumDecl(EnumDecl *D) {
+ if (Cfg.InlayHints.EndDefinitionComments &&
+ D->isThisDeclarationADefinition()) {
+ StringRef DeclPrefix;
----------------
zyounan wrote:
> nit: bail out early?
>
No longer needed as merged into VisitTagDecl
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:777
+ void addEndDefinitionCommentHint(const NamedDecl &D, StringRef DeclPrefix,
+ bool SkipSemicolon) {
+ size_t SkippedChars = 0;
----------------
sammccall wrote:
> SkipSemicolon doesn't need to be optional, a trailing semicolon never adds any ambiguity about what the hint is for
Yes, no ambiguity. But the following hint looks weird to me:
```
void foo() {
}; // foo
```
Since this isn't that complicated to filter them out, I'd prefer making it more precise.
================
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();
----------------
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).
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:797
+ // differently.
+ assert(Label.empty());
+ Label += printName(AST, D);
----------------
zyounan wrote:
> nit: `assert(Label.empty() && "Label should be empty with FunctionDecl")`. might be helpful for debugging.
No longer needed as this assert is removed.
================
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:
> 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.
================
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);
----------------
sammccall wrote:
> 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.
Now targeting
1. "}" if this is the only character
2. "};" or "} ;" or alike for enum/class/struct
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:772
+ Label = printName(AST, D);
+ } else {
+ // We handle type and namespace decls together.
----------------
sammccall wrote:
> sammccall wrote:
> > 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.
> >
> this is not quite done: we're still passing in the NamedDecl which offers the opportunity to go digging back into the class hierarchy. I think it's better to pass the name and source range in directly to make it clear we're not doing that.
Done
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1612
+ ExpectedHint{" // method3", "method3"},
+ ExpectedHint{" // Test::method2", "method2"},
+ ExpectedHint{" // Test::method4", "method4"});
----------------
sammccall wrote:
> I think we *probably* just want method2 here: showing qualifiers seems inconsistent with how we otherwise prefer brevity over disambiguation (e.g. not showing params etc)
I disagree with this and think the current behavior is consistent. The name we show is exactly the same with what is used to define the symbol. Also, I think the type name is an essential part of a method name.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1716
+
+ // No hint becaus trailing ';' is only allowed for class/struct/union/enum
+ void foo() {
----------------
zyounan wrote:
> typo: `becaus` -> `because`?
Good catch!
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1566
+
+TEST(EndDefinitionHints, Functions) {
+ assertEndDefinitionHints(R"cpp(
----------------
sammccall wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > can you add testcases where:
> > > - the function name is an operator
> > > - the function is templated
> > Added template function. I don't quite understand what you mean by "the function name is an operator". I'm assuming you meant operator overload so closing this one as covered below.
> oops, indeed it is - can you move one of the operator tests into "methods" and add a non-method one to "functions"?
> (Operators aren't really different enough to be separated out, I think)
Merged test cases
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1580
+
+TEST(EndDefinitionHints, Methods) {
+ assertEndDefinitionHints(R"cpp(
----------------
sammccall wrote:
> daiyousei-qz wrote:
> > sammccall wrote:
> > > 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)`.
> > I would argue we don't need hint for a lambda. This hint is only added to long definitions that may introduce a symbol. It is helpful to highlight the boundaries of different declarations. Because of the implementation assumes the hint is added to a `NamedDecl`, lambda should be safe.
> Up to you, I think a hint would be helpful but no hint is also fine.
>
> In any case, please add a test showing the behavior.
>
> > This hint is only added to long definitions that may introduce a symbol
>
> FWIW I don't think this is the greatest value from the hint - rather that *when the scope changes* we get some extra context that the local syntax doesn't provide. (This is why I think lambdas, for loops etc should eventually be hinted)
I agree, but the threshold of showing such hint should probably be more conservative since the number could blow up really quickly. I definitely think a hint for a long long if/for/while statement could be very helpful to understand the code structure.
Added test case for lambda. Close this for now since it's not going to be resolved in this patch.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1617
+ 0, ExpectedHint{" /* operator+ */ ", "opAdd"},
+ ExpectedHint{" /* operator bool */ ", "opBool"},
+ ExpectedHint{" /* operator int */ ", "opInt"});
----------------
daiyousei-qz wrote:
> sammccall wrote:
> > 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.
> I agree we should avoid very long hints being displayed. Instead of a length cutoff on type, however, I suppose a limit on the entire hint would be much simpler to implement. Do you think that's okay?
Added a max length limit of 50 for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150635/new/
https://reviews.llvm.org/D150635
More information about the cfe-commits
mailing list