sammccall added inline comments.
Herald added a project: clang-tools-extra.
================
Comment at: clang-tools-extra/clangd/Diagnostics.h:58
+ /// If true, Clangd will populate the data field in LSP diagnostic
+ /// representation. This is used to prevent extra data transfer with old
+ /// clients that doesn't support data field.
----------------
Second sentence is confusing because of inverted sense. And really I don't
think the reason is that we don't want to send extra data, but rather the fear
that old clients will choke on it.
If we're *not* afraid of that, i.e. we think they'll just drop it on the floor,
then I don't think we should bother to feature-detect it just so *we* can drop
it on the floor.
Not sure how you feel about this, but it's pretty tempting to me...
================
Comment at: clang-tools-extra/clangd/Diagnostics.h:82
+ // list.
+ std::vector<llvm::json::Value> FeatureModuleData;
};
----------------
Maybe call this OpaqueData or Data?
I like the comment but I'm not sure where this is ultimately set is so
important at this level (and we might use it in other ways!)
================
Comment at: clang-tools-extra/clangd/Protocol.h:853
+ /// and`textDocument/codeAction` request.
+ /// For clangd, this is always an array, with possibly zero elements.
+ llvm::json::Array data;
----------------
The array representation seems a bit awkward in practice: it's ~always going to
have 1 element (or none), so it seems like unnecessary wrapping.
I think the "natural" representation is something like `{"fooTweakParam": 42}`
or `{"fooTweakParams": {"x": 1, "y": 2}}`.
This array prevents tweaks from clobbering each other but OTOH it means the
data they read doesn't match the data they write. I think making data an Object
and passing in a mutable ref to it will work fine in practice.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98505/new/
https://reviews.llvm.org/D98505
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
