[Lldb-commits] [PATCH] D24369: [support] copying JSON parser/writer from lldb

[Mike Aizatsky via lldb-commits]

aizatsky abandoned this revision.

================
Comment at: include/llvm/Support/JSON.h:207
@@ +206,3 @@
+private:
+  typedef std::map<std::string, JSONValue::SP> Map;
+  typedef Map::iterator Iterator;
----------------
zturner wrote:
> `DenseMap` seems like a better choice here.
DenseMap doesn't seem to be defined for std::string keys.

================
Comment at: include/llvm/Support/JSON.h:238
@@ +237,3 @@
+
+  JSONValue::SP getObject(Index I);
+
----------------
zturner wrote:
> How about providing an overloaded `operator[]` and providing `begin()` and `end()` functions so that it can be used in a range based for syntax?
done for operator[].

begin() and end() will be tricky, because underlying storage has std::unique_ptr<>

================
Comment at: include/llvm/Support/JSON.h:282-283
@@ +281,4 @@
+
+  std::string Buffer;
+  size_t Index;
+};
----------------
zturner wrote:
> Instead of having a `std::string` here, which will copy the input JSON, I would make this a `StringRef`.  Then you can delete the `Index` field as well, because the internal `StringRef` itself could always point to the beginning of the next char by using a `Buffer = Buffer.drop_front(N)` like pattern.  This won't work if you ever have to do back-referencing across functions, but I don't think that is needed.  Also if `Buffer` is a `StringRef`, a lot of the methods like `peekChar()` and `skipSpaces` become trivial one-liners that you wouldn't even need to provide a separate function for.
`std::string => StringRef` - done.

As for removing `Index` - I'm not sure. It is currently used in error reporting and is the only feedback we give when we encounter an error.

================
Comment at: lib/Support/JSON.cpp:53
@@ +52,3 @@
+  case DataType::Signed:
+    return (uint64_t)Data.Signed;
+  case DataType::Double:
----------------
zturner wrote:
> Can you use C++ style casting operators here and in other similar functions?
Removed all 3 them and added get<T> method.


https://reviews.llvm.org/D24369