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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
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;
----------------
Is this `mutable` here also required for "cheating"?


================
Comment at: include/llvm/Support/JSON.h:355
+
+  // "cheating" move-constructor for moving from initializer_list.
+  ObjectKey(const ObjectKey &&V) {
----------------
Can you elaborate on why this is needed? AFAIK `std::initializer_list`s are not meant to be moved from.


================
Comment at: lib/Support/JSON.cpp:327
+  default:
+    if (isNumber(C)) {
+      double Num;
----------------
The error message we get seem to be:
If the token starts with an 'e' or 'E', the error we get is "Invalid number".
For the letters 'n', 't', or 'f' we get "Invalid bareword".
For any other first letter we get "Expected JSON value".

I'd hope for more consistent error messages.


================
Comment at: lib/Support/JSON.cpp:614
+  if (Options.getAsInteger(/*Radix=*/10, IndentAmount))
+    assert(false && "json::Value format options should be an integer");
+  unsigned IndentLevel = 0;
----------------
Did you consider using `llvm_unreachable`?


Repository:
  rL LLVM

https://reviews.llvm.org/D45753