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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
