[PATCH] D40596: [clangd] New conventions for JSON-marshalling functions, centralize machinery

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thanks you very much for this patch, JSON parsing now looks better than ever!
LGTM. (Just a few minor NITs).



================
Comment at: clangd/JSONExpr.h:71
+// The convention is to have a deserialize function findable via ADL:
+//     deserialize(const json::Expr&, T&)->bool
+// Deserializers are provided for:
----------------
The name `deserialize` may be a bit too general and prone to clashes with other code. Maybe choosing something a bit more specific like `json_deserialize` is a better option.
On the other hand, we do the SFINAE trick to check the types are correct in `Expr` constructor, so we should be fine either way.


================
Comment at: clangd/JSONExpr.h:512
+  if (auto *O = E.asObject()) {
+    for (const auto &KV : *O)
+      if (!deserialize(KV.second, Out[llvm::StringRef(KV.first)]))
----------------
Maybe also call `Out.clear()` to be consistent with `deserialize(..., vector)`?


================
Comment at: clangd/JSONExpr.h:547
+
+  const json::obj *O;
+};
----------------
Maybe make this field private?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40596