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

[Sam McCall via Phabricator via llvm-commits]

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

In https://reviews.llvm.org/D45753#1076478, @chandlerc wrote:

> In https://reviews.llvm.org/D45753#1075648, @sammccall wrote:
>
> > @chandlerc: any interest in reviewing this JSON library (either at a high level for suitability in `llvm/Support`, or for detailed review)?
>
>
> I'm happy to tackle both of these. I'll want to do a bit of homework to make sure that the high level concerns that came up previously are adequately addressed (or there is some plan to address them).


Thank you! Other than "how does this relate to YAML", I don't think I'm familiar with the concerns so any pointers you have would be appreciated.



================
Comment at: include/llvm/Support/JSON.h:8
+//
+//===---------------------------------------------------------------------===//
+
----------------
chandlerc wrote:
> One thing that would help me even as I start to dig into this would be an overview comment at the top of the file...
> 
> - What is the intended / expected usage pattern?
> - Mention alternatives given that it seems unlikely we'll eliminate YAMLIO immediately. Why use one vs. the other?
> 
> 
> I'd also like to see your initial thoughts on why this library is better as a separate API/library rather than a separate interface that is part and parcel of the YAML library we already have. I don't have any real opinion (yet) about what does or doesn't make sense, and having your perspective on this would help me form an opinion I suspect.
Oops, the big comment on Value was meant to serve this purpose, but I can't find a way to get it to be first in the file.
Added a real file overview - right level of detail?

There's a few reasons to have this separate from the YAML library, and I'm not sure which are important or even good.
 - a YAML based dom/parser will have a poor API for people who want to deal with JSON - it's too complicated and the names are incorrect. The thing I like most about this API is that the usages are simple.
 - neither the YAMLParser nor YAML I/O design is useful for the use cases I've run into so far (e.g. parsing LSP correctly really requires a DOM, YAMLParser is too low-level and YAML I/O is too restrictive). There'd be a lot of work in filling out the format x API matrix to make it coherent. Maybe it's worth it, so far it could be YAGNI.
 - I'm trying to avoid becoming a YAML expert myself, it's complicated and IMO a dead-end.
 - I simply don't have good ideas about how to combine the pieces I want to add into a single coherent API that I'd want to use. How should YAML anchor data be represented in a DOM? What happens if we try to write JSON-incompatible data to a JSON stream? How can we make YAML I/O more flexible without making it yet more complicated? I'm sure these are solvable, but they might need to be solved by someone who's more in tune with the goals of the YAML library.



================
Comment at: include/llvm/Support/JSON.h:26-27
+
+// An Object is a JSON object, which maps strings to heterogenous JSON values.
+// The string keys may be owned or references.
+class Object : public std::map<ObjectKey, Value> {
----------------
chandlerc wrote:
> Feel free to defer this until higher level stuff is addressed, but here and throughout this entire file, you have excellent comments but don't use a doxygen comment prefix. I think all of these should be converted to be actual doxygen comments at whatever point this is moving forward.
Ack, I'll do this conversion shortly.


================
Comment at: include/llvm/Support/JSON.h:313-316
+  mutable ValueType Type;
+  mutable llvm::AlignedCharArrayUnion<bool, double, llvm::StringRef,
+                                      std::string, json::Array, json::Object>
+      Union;
----------------
Meinersbur wrote:
> Is this `mutable` here also required for "cheating"?
Yes. This is documented at `moveFrom`, but added another comment.


================
Comment at: include/llvm/Support/JSON.h:355
+
+  // "cheating" move-constructor for moving from initializer_list.
+  ObjectKey(const ObjectKey &&V) {
----------------
Meinersbur wrote:
> Can you elaborate on why this is needed? AFAIK `std::initializer_list`s are not meant to be moved from.
I was able to mitigate this, eliminating the const-rvalue-reference constructors. (This optimization isn't important for ObjectKey, and for Object I friended the relevant classes instead.)

The reasoning here is roughly:
 - we need some syntax to support many KV pairs for an object, or elements in an array
 - function syntax fails because it doesn't format well in long-list cases. Clang-format does a better job if lists are braced lists, in particular it offers you the ability to force one-per-line with a trailing comma in the list.
 - a variadic constructor fails because this must be templated on the arg type, which means args can't be braced list expressions themselves, as those do not have a deducible type. This would hurt map-like object-literal syntax...
 - so we're left with the `std::initializer_list` constructor as the way to pass variable numbers of arguments, in a way that formats nicely, and allows them to be coerced to a chosen type. However `initializer_list` acts like a container of `const<T>`, which would mean naively json::Value{{{{{1}}}}} would result in a deep copy at every level of nesting. Fortunately the standard spells out enough of how the contents of init-lists are constructed that moving the data out of them seems well-defined.

I'm not sure how much of this stuff belongs in the comments here - it's more design doc than user guide.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753