[PATCH] D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.

[Sam McCall via Phabricator via llvm-commits]

sammccall marked 8 inline comments as done.
sammccall added a comment.

Thanks a lot for the review!
Made most of the changes. The one I'm least certain about: I didn't move `json::parse` --> `parseJSON`.
As you suggest, going to land this patch and happy to make any followup changes, I don't expect much API churn (even `parse` doesn't tend to have a lot of callsites).



================
Comment at: include/llvm/Support/JSON.h:292
+      typename = typename std::enable_if<std::is_arithmetic<T>::value>::type,
+      typename = typename std::enable_if<std::integral_constant<
+          bool, !std::is_same<T, bool>::value>::value>::type>
----------------
chandlerc wrote:
> Does some compiler reject this as just `std::enable_if<!std::is_same<T, bool>::value>::type`? I'd guess MSVC might complain about the use of ! but that would make me sad.
That works fine. I just didn't look critically enough at this once it compiled :-)


================
Comment at: include/llvm/Support/JSON.h:474-477
+struct Object::KV {
+  ObjectKey K;
+  Value V;
+};
----------------
chandlerc wrote:
> Consider inlining this above? I actually think it makes the `std::initializer_list<KV>` used in a public API much easier to read if this trivial wrapper is immediately visible.
> 
> Oh, I guess this is kept out-of-line in order to define `ObjectKey` after `Object`, `Array`, and `Value`?
> 
> Not sure this ordering is buying that much in terms of readability. Given that you can define `ObjectKey` first, I might just do that and avoid the need to make this out-of-line... Anyways, this is just an optional suggestion, I'm fine with whichever way you end up.
I wish this was just readability :-(

The problem is a circular dependency: defining `KV` needs `Value` to be complete, `Value` requires `Object` to be complete (because of `Union`). So the `Object` -> `Value` -> `KV` ordering is necessary, I think.

`ObjectKey` could indeed be hoisted higher, but that alone doesn't buy anything, I think.


================
Comment at: include/llvm/Support/JSON.h:490-491
+
+// Standard deserializers are provided for primitive types.
+// See comments on Value.
+inline bool fromJSON(const Value &E, std::string &Out) {
----------------
chandlerc wrote:
> Given that these have `JSON` in the name, does it make sense to move them out of the `json` namespace?
I'm not sure. The intent is these are part of a family of functions found via ADL on the second argument.
When used in that way, they are always available (because of ADL on the first argument), and they don't particularly "belong "in namespace llvm (they overload for builtin and `std` types).

Most of these particular functions are for generic code (like ObjectMapper, or `fromJSON(Value, std::vector)` itself) . e.g. calling `Value.getAsNumber()` is generally nicer than `fromJSON(Value, double)`. So moving these particular overloads into `namespace llvm` seems to give them more prominence compared to the rest of the API than seems warranted.

c.f. `llvm::json::parse` vs `llvm::parseJSON`, which *is* a main entrypoint.


================
Comment at: include/llvm/Support/JSON.h:554-565
+/// Helper for mapping JSON objects onto protocol structs.
+///
+/// Example:
+/// \code
+///   bool fromJSON(const Value &E, MyStruct &R) {
+///     ObjectMapper O(E);
+///     if (!O || !O.map("mandatory_field", R.MandatoryField))
----------------
chandlerc wrote:
> This is really nice btw. =D
:-) We had plenty of this code in clangd to test the API on.

The one thing that's pretty awful is that when parsing fails there's no detailed representation of the error. But worse seems to be better for us so far.


================
Comment at: include/llvm/Support/JSON.h:596-599
+/// Parses the provided JSON source, or returns a ParseError.
+/// The returned Value is self-contained and owns its strings (they do not refer
+/// to the original source).
+llvm::Expected<Value> parse(llvm::StringRef JSON);
----------------
chandlerc wrote:
> Would it be a readability improvement to have this be `llvm::parseJSON` rather than `llvm::json::parse`?
Hmm, this seems like a wash to me. `parseJSON` is slightly shorter than `json::parse`, while the latter hints (accurately) at a library boundary.

Given the *other* entrypoints (`Value`, `ObjectMapper` etc) are in `namespace json`, I lean towards keeping this there too - among other things, it makes the spelling/capitalization of `json::parse`, `json::Value` etc easier to remember I think.

Certainly happy to change this if you have a strong opinion here though.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753