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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits