[PATCH] D98505: [clangd] Propagate data in diagnostics

[Sam McCall via Phabricator via cfe-commits]

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