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

[David Blaikie via lldb-commits]

On Fri, Sep 9, 2016 at 7:31 PM Mike Aizatsky <aizatsky at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160910/d52ff950/attachment.html>