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