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