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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I'm not sure who should be the main person reviewing this, but I think the implementation looks pretty good, and would be a great replacement for the one we have in lldb. The main thing I noticed is that you seem to be rolling your own utf8 parser -- I would hope we can reuse the existing unicode utilities here.

> fromJSON returns only bool. A richer error-signaling mechanism may be useful to provide useful messages, or let recursive fromJSONs (containers/structs) do careful error recovery.

For our (lldb) use case, we don't need a fancy error mechanism. It seems it should be possible to make these return `llvm::Error` if it turns out to be necessary later.

> should json::obj be always explicitly written (like json::ary)

I don't have an opinion on that, though I do think that `json::ary` should be renamed.

> there's no streaming parse API. I suspect there are some simple wins like a callback API where the document is a long array, and each element is small. But this can probably be bolted on easily when we see the need.

All the messages we are parsing are reasonably small (<= 1k), so we don't have a need for a streaming parser right now. The parser is essentially streaming already. If there was something like llvm::raw_istream, it could be trivially converted, but it looks like input streams aren't a thing for llvm in general, so I wouldn't be worried about it here.



================
Comment at: include/llvm/Support/JSON.h:58
+// Array and Object also have typed indexing accessors for easy traversal:
+//   Expected<Expr> E = parse(R"( {"options": {"font": "sans-serif"}} )");
+//   if (json::obj* O = E->asObject())
----------------
It would be good to emphasize that the returned object's lifetime is independent of the string being parsed (i.e. the contained strings are not references to the strings in the original text).


================
Comment at: include/llvm/Support/JSON.h:449-450
+// Give Expr::{Object,Array} more convenient names for literal use.
+using obj = Expr::ObjectExpr;
+using ary = Expr::ArrayExpr;
+
----------------
I find it a bit inconsistent when I see a `json::Expr` (uppercase) for the base type and then `json::obj` (lowercase) for the derived ones. The lowercase names don't really follow the naming convention and `ary` is also fairly un-obvious. Could we just call these `Object` and `Array` ?


================
Comment at: lib/Support/JSON.cpp:283-364
+void Parser::encodeUtf8(uint32_t Rune, std::string &Out) {
+  if (Rune <= 0x7F) {
+    Out.push_back(Rune & 0x7F);
+  } else if (Rune <= 0x7FF) {
+    uint8_t FirstByte = 0xC0 | ((Rune & 0x7C0) >> 6);
+    uint8_t SecondByte = 0x80 | (Rune & 0x3F);
+    Out.push_back(FirstByte);
----------------
Is there any reason `Support/ConvertUTF.h` cannot be used here? Is sounds like you just need the "lenient" conversion mode here.


================
Comment at: lib/Support/JSON.cpp:394
+
+namespace {
+void quote(llvm::raw_ostream &OS, llvm::StringRef S) {
----------------
llvm coding standards say we use static for functions instead of anonymous namespaces. Also, the llvm and json namespaces are opened and closed twice, so this may be a good opportunity to merge them.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753