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

Sam McCall via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 27 06:31:01 PDT 2018


sammccall added inline comments.


================
Comment at: include/llvm/Support/JSON.h:315
+  /// For pretty-printing, use the formatv() format_provider below.
+  friend llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Value &);
+
----------------
simon_tatham wrote:
> When I built this locally, I had a strange build failure involving this function with g++ 5.4.0 (i.e. the default compiler on Ubuntu 16.04). It reported, at the definition of this function in `JSON.cpp`, this error:
> ```
> error: ‘llvm::raw_ostream& llvm::json::operator<<(llvm::raw_ostream&, const llvm::json::Value&)’ should have been declared inside ‘llvm::json’
> ```
> for which, apparently, the fix was to add a repeated declaration of this function without the 'friend' keyword and outside the definition of `class Value`.
> 
> clang was happy with it, on the other hand. I've no idea :-)
Indeed, thanks for catching this.
I think GCC is technically correct here, and most other compilers prefer to be helpful instead :-)
Added the namespace-scope declaration.


================
Comment at: lib/Support/JSON.cpp:560
+    case T_Number:
+      OS << format("%g", as<double>());
+      break;
----------------
simon_tatham wrote:
> Could this number formatting be changed? The default %g loses precision – you don't even get enough information to exactly reconstruct the same double you started with.
> 
> Also, over in D46054 I'm working on a JSON back end for TableGen, for which I'd find it useful to be able to pass an arbitrary 64-bit integer through this system and still have the full 64 bits of integer value visible in the JSON output file, for the benefit of JSON consumers (e.g. Python `json.load`) that go above the call of duty in returning it as an integer without rounding it to the nearest representable double.
> 
> So, would it be possible to have some method of constructing a json::Value that formats as a 64-bit integer literal?
> Could this number formatting be changed? The default %g loses precision – you don't even get enough information to exactly reconstruct the same double you started with.

Yes, though is there an existing aware of round-trip safe double formatter in llvm?
I suspect this only actually matters when the values are integers, so we should consider your second suggestion first :-)

> I'd find it useful to be able to pass an arbitrary 64-bit integer through this system and still have the full 64 bits of integer value visible in the JSON output file, for the benefit of JSON consumers (e.g. Python json.load) that go above the call of duty in returning it as an integer without rounding it to the nearest representable double.

What about this design:
- Internally, a numeric value can be an integer or a double. i.e we split the internal `ValueType` `T_Number` into two, `T_Integer` and `T_Double`. public `Kind` remains unchanged.
- when constructing, you get one or the other depending on the static type
- when parsing, you get integer unless it has a nonzero decimal part or is out-of-range.
- `asDouble()` always succeeds
- `asInteger()` succeeds if the underlying value is integer or if it's a double that can be exactly represented as `int64_t` (same as now)
- when serializing, you get `%g` for double and the usual representation for integers

Open questions:
- is 1.2e3 a double or an integer? I kind of want the former, which complicates our heuristic.
- `int64_t` leaves anyone who wants `uint64_t` out in the cold. But adding more options for types is going to lead to madness. Can we live with this limitation?

If this sounds good I can start on the changes, but I'd like to defer adding new features to another patch if that's OK. This one is largely moving mostly-battleworn code from clangd, and new features need closer review of the implementation.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753





More information about the llvm-commits mailing list