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

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: include/llvm/Support/JSON.h:27
@@ +26,3 @@
+public:
+  typedef std::shared_ptr<JSONValue> SP;
+  enum class Kind { String, Number, True, False, Null, Object, Array };
----------------
I scanned the entire patch and I'm not sure I understand the need for the covariant typedefs.  The only type that ever seems to be used is `JSONValue`, so it seems to me like the other types are unnecessary.

Also, I'm not sure `shared_ptr` is necessary.  Wouldn't `unique_ptr` work just fine?  Then you could vend references to values instead of pointers.

================
Comment at: include/llvm/Support/JSON.h:47
@@ +46,3 @@
+  JSONString(const char *S);
+  JSONString(const std::string &S);
+
----------------
I would make this a `StringRef`.  Currently if someone calls this with a `StringRef`, it will make a copy.  If you change the constructor to take a `StringRef`, then it won't take a copy regardless of whether it's called with a `StringRef` or a `std::string`.

================
Comment at: include/llvm/Support/JSON.h:56
@@ +55,3 @@
+
+  std::string getData() const { return Data; }
+
----------------
I would return a `StringRef` here.  Otherwise you're guaranteed to make a copy, if you return a `StringRef` it will only make a copy when you assign it to a `std::string`.

================
Comment at: include/llvm/Support/JSON.h:63
@@ +62,3 @@
+private:
+  static std::string jsonStringQuoteMetachars(const std::string &);
+
----------------
This could be a file static in the .cpp file, and also it could take a `StringRef` instead of a `const std::string &`.

================
Comment at: include/llvm/Support/JSON.h:202
@@ +201,3 @@
+
+  JSONValue::SP getObject(const std::string &Key);
+
----------------
Couple things here.

1. `Key` could be a `StringRef` 
2. This method should probably be const.  
3. You could make the underlying value a `unique_ptr` instead of a `shared_ptr` and return a reference.  You could also have a `tryGetValue` in cases where you aren't sure if the value exists.  Not sure what would be better.

================
Comment at: include/llvm/Support/JSON.h:207
@@ +206,3 @@
+private:
+  typedef std::map<std::string, JSONValue::SP> Map;
+  typedef Map::iterator Iterator;
----------------
`DenseMap` seems like a better choice here.

================
Comment at: include/llvm/Support/JSON.h:229-231
@@ +228,5 @@
+  typedef std::vector<JSONValue::SP> Vector;
+  typedef Vector::iterator Iterator;
+  typedef Vector::size_type Index;
+  typedef Vector::size_type Size;
+
----------------
I don't know if the `typedefs` here help or hurt readability.  I would just make them `size_t`, and remove the `Iterator` typedef as it doesn't seem to be used.

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

================
Comment at: include/llvm/Support/JSON.h:282-283
@@ +281,4 @@
+
+  std::string Buffer;
+  size_t Index;
+};
----------------
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.

================
Comment at: lib/Support/JSON.cpp:19
@@ +18,3 @@
+
+std::string JSONString::jsonStringQuoteMetachars(const std::string &S) {
+  if (S.find('"') == std::string::npos)
----------------
Input parameter should be a `StringRef`.

================
Comment at: lib/Support/JSON.cpp:23
@@ +22,3 @@
+
+  std::string Output;
+  const size_t Size = S.size();
----------------
Should probably do an `Output.reserve(S.size());`

================
Comment at: lib/Support/JSON.cpp:24-26
@@ +23,5 @@
+  std::string Output;
+  const size_t Size = S.size();
+  const char *Chars = S.c_str();
+  for (size_t I = 0; I < Size; I++) {
+    unsigned char Ch = *(Chars + I);
----------------
`for (char CH : S)`

================
Comment at: lib/Support/JSON.cpp:53
@@ +52,3 @@
+  case DataType::Signed:
+    return (uint64_t)Data.Signed;
+  case DataType::Double:
----------------
Can you use C++ style casting operators here and in other similar functions?

================
Comment at: lib/Support/JSON.cpp:115-116
@@ +114,4 @@
+  OS << '{';
+  auto Iter = Elements.begin(), End = Elements.end();
+  for (; Iter != End; Iter++) {
+    if (First)
----------------
```
for (const auto &I : Elements) {
```

================
Comment at: lib/Support/JSON.cpp:187
@@ +186,3 @@
+
+JSONArray::Size JSONArray::getNumElements() { return Elements.size(); }
+
----------------
This function should be `const`.

================
Comment at: lib/Support/JSON.cpp:283
@@ +282,3 @@
+  case '9': {
+    uint64_t ExpIndex = 0;
+    bool Done = false;
----------------
Don't like the idea of reimplementing string -> number parsing, but I'm not sure if LLVM has something to solve this for us.


https://reviews.llvm.org/D24369