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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
