sammccall added a comment. This looks like the right approach to me, thanks for doing this!
What do you want to do about paddingLeft/paddingRight? IIUC where we're currently sending `label="foo: "` we need to send `label="foo:", paddingRight=true`. But updating these strings might have a blast radius in the tests, so it might be best to do it in a separate patch? ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:586 + // Only advertise inlayHints extension if client doesn't support the standard + // implementation. ---------------- I still feel a little uncomfortable with this because it cuts against the design of capabilities, and it's not clear what concrete problem it solves. Most likely it won't cause nor solve any problems. But it might lead to a lot of confusion. (e.g. in nvim and some others, base capabilities are provided but customized by extensions/users. If one extension sets the standard capability and another tries to use clangd hints, or the user sets the wrong capability at first out of confusion, it seems hard to debug) However it's up to you, I promise this is my last comment about it :-) ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1222 + // We have a "range" property and "kind" is represented as a string, not as an + // enum value. + auto Serialize = [Reply = std::move(Reply)]( ---------------- link to https://clangd.llvm.org/extensions#inlay-hints ? ================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1232 + for (auto &Hint : *Hints) { + llvm::json::Object Current(*toJSON(Hint).getAsObject()); + Current["kind"] = llvm::to_string(Hint.kind); ---------------- Defining the extension JSON representation as a delta of the standard JSON representation seems brittle and it's hard to tell what the result is. It doesn't seem like much code to encode Hint explicitly? ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:1322 static llvm::StringLiteral toString(InlayHintKind K) { switch (K) { ---------------- this can now be inlined into operator<< again, it's not being called anywhere else ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:1337 + if (H.kind == InlayHintKind::Type || H.kind == InlayHintKind::Parameter) + Result["kind"] = static_cast<int>(H.kind); + return Result; ---------------- We can't serialize our extension kinds, a new version of the protocol might come out that says 3 means nullability hints or something and existing versions of clangd are sending incorrect kinds. (probably the most regular thing to do is define `json::Value toJSON(InlayHintKind)` that returns int | null, but up to you) ================ Comment at: clang-tools-extra/clangd/Protocol.h:497 + + /// Whether the client supports inlayHints requests. This is tracked purely to + /// prevent clangd from advertising it's protocol extension, which predates ---------------- nit: inlayHint, not hints (mostly confusing because arity differs between the two versions, and this is the wrong one) ================ Comment at: clang-tools-extra/clangd/Protocol.h:1536 enum class InlayHintKind { - /// The hint corresponds to parameter information. - /// An example of a parameter hint is a hint in this position: - /// func(^arg); - /// which shows the name of the corresponding parameter. - ParameterHint, - /// The hint corresponds to information about a deduced type. /// An example of a type hint is a hint in this position: ---------------- we almost always use the comments from LSP in this file. they're very often borderline useless, but there's some value in consistency. (And in the case of InlayHint, some of the differences are meaningful) ================ Comment at: clang-tools-extra/clangd/Protocol.h:1573 /// The range allows clients more flexibility of when/how to display the hint. Range range; ---------------- this is an (unserialized) clangd extension ================ Comment at: clang-tools-extra/clangd/Protocol.h:1580 /// /// The label may contain punctuation and/or whitespace to allow it to read /// naturally when placed inline with the code. ---------------- this is no longer correct: whitespace should no longer be included and paddingLeft/paddingRight should be set instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125228/new/ https://reviews.llvm.org/D125228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
