<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 9, 2016 at 7:31 PM Mike Aizatsky <<a href="mailto:aizatsky@google.com">aizatsky@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aizatsky abandoned this revision.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/Support/JSON.h:207<br class="gmail_msg">
@@ +206,3 @@<br class="gmail_msg">
+private:<br class="gmail_msg">
+  typedef std::map<std::string, JSONValue::SP> Map;<br class="gmail_msg">
+  typedef Map::iterator Iterator;<br class="gmail_msg">
----------------<br class="gmail_msg">
zturner wrote:<br class="gmail_msg">
> `DenseMap` seems like a better choice here.<br class="gmail_msg">
DenseMap doesn't seem to be defined for std::string keys.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/Support/JSON.h:238<br class="gmail_msg">
@@ +237,3 @@<br class="gmail_msg">
+<br class="gmail_msg">
+  JSONValue::SP getObject(Index I);<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
zturner wrote:<br class="gmail_msg">
> How about providing an overloaded `operator[]` and providing `begin()` and `end()` functions so that it can be used in a range based for syntax?<br class="gmail_msg">
done for operator[].<br class="gmail_msg">
<br class="gmail_msg">
begin() and end() will be tricky, because underlying storage has std::unique_ptr<><br class="gmail_msg"></blockquote><div><br></div><div>see llvm::pointee_iterator in include/llvm/ADT/iterator.h</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/Support/JSON.h:282-283<br class="gmail_msg">
@@ +281,4 @@<br class="gmail_msg">
+<br class="gmail_msg">
+  std::string Buffer;<br class="gmail_msg">
+  size_t Index;<br class="gmail_msg">
+};<br class="gmail_msg">
----------------<br class="gmail_msg">
zturner wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
`std::string => StringRef` - done.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Support/JSON.cpp:53<br class="gmail_msg">
@@ +52,3 @@<br class="gmail_msg">
+  case DataType::Signed:<br class="gmail_msg">
+    return (uint64_t)Data.Signed;<br class="gmail_msg">
+  case DataType::Double:<br class="gmail_msg">
----------------<br class="gmail_msg">
zturner wrote:<br class="gmail_msg">
> Can you use C++ style casting operators here and in other similar functions?<br class="gmail_msg">
Removed all 3 them and added get<T> method.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D24369" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D24369</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>