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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: omtcyfz.

Ok, I generally think this is looking good to go in... As mentioned previously, I think there is a good rationale around supporting this despite the existing other/similar libraries.

The code quality is crazy high. Awesome job there.

I have a bunch of pretty minor questions or suggestions below. But I think they're mostly optional stuff. Even if you decide to make the changes, I'm even fine with those being follow-up patches and such as they seem unlikely to wildly change the API surface here, and none of them seem to represent serious bugs or anything.

So don't block on any of this to land the patch and move to a more iterative model. The patch as a whole is LGTM, and I'd just suggest folding the suggestions below that make sense to you to fold into the initial one you land, and follow-up on anything else.



================
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>
----------------
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.


================
Comment at: include/llvm/Support/JSON.h:353
+      double D = as<double>();
+      if (LLVM_LIKELY(std::modf(D, &D) == 0 &&
+                      D >= std::numeric_limits<int64_t>::min() &&
----------------
I would write this as `std::modf(D, &D) == 0.0`. The return is a double, and this way it looks less like you're checking for the absence of an error code and more like actually checking the fractional component is zero.



================
Comment at: include/llvm/Support/JSON.h:354-355
+      if (LLVM_LIKELY(std::modf(D, &D) == 0 &&
+                      D >= std::numeric_limits<int64_t>::min() &&
+                      D <= std::numeric_limits<int64_t>::max()))
+        return D;
----------------
I would explicitly convert the RHS of these two to a double to make it clear that this comparison takes place is double precision floating point, not in an integral type. I think that would help the reader out, because otherwise it could look a bit like a tautology.


================
Comment at: include/llvm/Support/JSON.h:396
+  template <typename T, typename... U> void create(U &&... V) {
+    new (&as<T>()) T(std::forward<U>(V)...);
+  }
----------------
Sadly, I think this is invalid by the pedantic wording of the spec.

Because `as<T>()` binds a reference to the pointer, I think that the pointer has to point to a valid object. And it doesn't yet...

I would just inline the `reinterpret_cast` here despite that repetition because I think all other users of `as<T>()` are correct.

However, since `as<T>()` is a private implementation detail, you could alternatively just return the pointer. Really up to you.


================
Comment at: include/llvm/Support/JSON.h:458-460
+  friend bool operator<(const ObjectKey &L, const ObjectKey &R) {
+    return L.Data < R.Data;
+  }
----------------
Sink this to be a non-friend operator like operator `==` and `!=`?

Or hoist those to be friend operators?

I don't have a strong opinion about one pattern or the other, mostly just advocating for consistency whichever way you want to go.


================
Comment at: include/llvm/Support/JSON.h:463-464
+private:
+  std::unique_ptr<std::string> Owned;
+  llvm::StringRef Data;
+};
----------------
Are there likely to be a lot of these? Might make sense to leave a note for the future that this could be optimized a lot by having a custom `StringRef` like implementation that encodes whether the data is owned w/o spending an extra pointer on it. Clearly not needed in this patch, just a thought for the future if it comes up.


================
Comment at: include/llvm/Support/JSON.h:474-477
+struct Object::KV {
+  ObjectKey K;
+  Value V;
+};
----------------
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.


================
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) {
----------------
Given that these have `JSON` in the name, does it make sense to move them out of the `json` namespace?


================
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))
----------------
This is really nice btw. =D


================
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);
----------------
Would it be a readability improvement to have this be `llvm::parseJSON` rather than `llvm::json::parse`?


Repository:
  rL LLVM

https://reviews.llvm.org/D45753