On Fri, Sep 9, 2016 at 7:31 PM Mike Aizatsky <aizat...@google.com> wrote:
> 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<> > see llvm::pointee_iterator in include/llvm/ADT/iterator.h > > ================ > 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
