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