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

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Looks like I forgot to clang-format before sending this :-/

To avoid adding spurious diffs during review, I'll do that before landing (if applicable!).



================
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> {
----------------
sammccall wrote:
> 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.
Used doxygen comment prefix and wrapped a long example in `\code...\endcode`.

Many comments apply to blocks of related functions so are not doxygenated.
For the most part no actual doxygen annotations seemed appropriate, and I think rewriting comments to make more use of doxygen often hurts the inline readability of the comments.

So I think this is done, but LMK if you disagree.


================
Comment at: lib/Support/JSON.cpp:327
+  default:
+    if (isNumber(C)) {
+      double Num;
----------------
Meinersbur wrote:
> 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.
There's a tension here between precise errors, consistent/useful errors, and parser complexity.
The "invalid number" is a false positive for `elephant` but a true positive for `123,00`.

I've renamed these messages to be consistent but with a hint: "Invalid JSON value (number?)", "Invalid JSON value (null?)" etc, and "Invalid JSON value" respectively - WDYT?


Repository:
  rL LLVM

https://reviews.llvm.org/D45753