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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 4 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:77
+  // list.
+  std::vector<llvm::json::Object> OpaqueData;
 };
----------------
sammccall wrote:
> Hmm, you've replaced the json::Array with an array of objects :-)
> Any reason we can't just use llvm::json::Object here?
> 
> We don't really handle conflicts anyway, and I don't think having one tweak read another one's data out of this field is a real concern.
oops, that wasn't the intention. changed to `llvm::json::Object`.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:616
+  if (auto *Data = Params.getAsObject()->getObject("data"))
+    R.data = std::move(*Data);
+  return O.map("range", R.range) && O.map("message", R.message) &&
----------------
sammccall wrote:
> std::move is a fancy way to spell copy here, since Params is const.
> 
> json::Value(*Data) (or can you just use mapOptOrNull?)
> std::move is a fancy way to spell copy here, since Params is const.

right.. dropped that.

> json::Value(*Data) (or can you just use mapOptOrNull?)

we can't `mapOptOrNull` as `fromJSON` doesn't have output specializations for JSON types. i don't think it is worth having them (i think, they would actually be confusing). but if you think it's worth, maybe we can have some identity mapper specialization.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:71
+    /// Diagnostics related to this selection.
+    llvm::ArrayRef<clangd::Diagnostic> Diags;
     // FIXME: provide a way to get sources and ASTs for other files.
----------------
sammccall wrote:
> unrelated?
> (well not quite, but neither populated or used in this patch)
yeah this was supposed to be a separate change but forgot to strip this bit off..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98505/new/

https://reviews.llvm.org/D98505