[PATCH] D87170: [json] Provide a means to delegate writing a value to another API

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

This is really nice. There's an argument for parsing and emitting (validation and formatting) but obviously also a performance tradeoff so the feature makes sense.

Just some style nits (mostly local consistency) and a small API tweak again for consistency.



================
Comment at: llvm/include/llvm/Support/JSON.h:793
     objectEnd();
   }
 
----------------
can we have a `rawValue(function_ref<void(raw_ostream&)>)`?

The "low-level" ideas of raw-output and no-lexical-nesting are independent.

Providing the ostream as a parameter makes the usable lifetime clear.


================
Comment at: llvm/include/llvm/Support/JSON.h:821
 
- private:
+  /// Inform the writer that something external is about to write a value to OS.
+  void rawValueBegin();
----------------
Consistently with the others, we should doc this among the high level functions, and leave the low-level ones undocumented.


================
Comment at: llvm/include/llvm/Support/JSON.h:821
 
- private:
+  /// Inform the writer that something external is about to write a value to OS.
+  void rawValueBegin();
----------------
sammccall wrote:
> Consistently with the others, we should doc this among the high level functions, and leave the low-level ones undocumented.
"something external" is a bit vague.

Maybe "Write an externally-serialized value. The caller must write exactly one valid JSON value to the provided stream. No validation or formatting is performed on the output."


================
Comment at: llvm/include/llvm/Support/JSON.h:822
+  /// Inform the writer that something external is about to write a value to OS.
+  void rawValueBegin();
+  /// Inform the writer that something external has finished writing a value to
----------------
This can return raw_ostream&, to relieve the caller of needing to hold a reference, and to hide the implementation detail that we don't internally wrap the original ostream.


================
Comment at: llvm/include/llvm/Support/JSON.h:842
+    /// Something outside this object is writing to OS such as a 3rd-party API.
+    ExternalStreamWriter,
   };
----------------
nit: this should match the name of the feature elsewhere, as they directly correspond. `RawValue`?
nit: comment needs to mention that it's a single value
nit: line comment as above.

Maybe "// External code is writing a single value to OS directly"?


================
Comment at: llvm/lib/Support/JSON.cpp:639
 
+void llvm::json::OStream::rawValueBegin() {
+  valueBegin();
----------------
nit: move after attributeBegin/End to match decls?


================
Comment at: llvm/lib/Support/JSON.cpp:647
+  assert(Stack.back().Ctx == ExternalStreamWriter &&
+         "Expected matching rawValueBegin");
+  Stack.pop_back();
----------------
nit: drop assertion message for consistency with other End functions


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87170/new/

https://reviews.llvm.org/D87170