Cleanup: Some simplifications in json_schema_compiler.

tools/json_schema_compiler/util.h

Index: tools/json_schema_compiler/util.h
diff --git a/tools/json_schema_compiler/util.h b/tools/json_schema_compiler/util.h
index 228ecedba6b5023a99ec66de5ba5a7abe0c8d506..5fda37516d210e20291e2bd0bd0f43b756b23179 100644
--- a/tools/json_schema_compiler/util.h
+++ b/tools/json_schema_compiler/util.h
@@ -16,23 +16,45 @@ namespace json_schema_compiler {
 
 namespace util {
 
+struct ITEMS_ARE_BINARY {};
+struct ITEMS_ARE_NOT_BINARY {};

[not at google - send to devlin, 2015/01/13 18:15:13]

Mhm ok I'm going to add Jeffrey here to advise on the most idiomatic way of
doing this. Perhaps in the new world there's an easy way to do all this, e.g.
without the structs and needing to instantiate them etc. Maybe a boolean? Maybe
C++11 provides an easy way?

For example, I did a bunch of playing, and it looks like you can avoid the icky
marker struct (and cut down a tiny bit on binary size by generating less code;
or at least, make it more readable) with something like:

template <typename OutType>
using GetItemFromListStrategy =
    bool(const base::ListValue&, int, OutType*);

// implementations of each strategy
bool GetIntFromList(
    const base::ListValue& list, int index, int* out);
bool GetDoubleFromList(
    const base::ListValue& list, int index, double* out);
...
bool GetBinaryFromList(
    const base::ListValue& list, int index, base::BinaryValue* out);

then

template <typename OutType>
bool PopulateArrayFromList(
    const base::ListValue& from,
    GetItemFromListStrategy<OutType> strategy,
    std::vector<OutType>* out) {
  ...  // that implementation
}

and then you'd generate slightly different code (more, but less magical), which
needs to pass in the strategy, but it avoids some templates?

But I'm not sure that's any better. I defer to Jeffrey.

[Jeffrey Yasskin, 2015/01/13 18:57:08]

Function pointers are generally slow to call, although we don't usually worry
much about performance in the extensions bindings, right?

The core problem here is that we want to store both UTF-8 strings and
ArrayBuffers in std::string? Would it make sense to store the ArrayBuffers into
a vector<unsigned char>, and then use the existing overloading to deal with it?

[not at google - send to devlin, 2015/01/13 19:39:56]

I'm fairly certain a lot more is going on than calling function pointers.
Another tradeoff is this vs binary size, but either way it's a miniscule effect
and shouldn't affect things. Code cleanliness is what I like optimising for.
Ah right, yeah, that's a good idea. It would be a really simple change on the
schema compiler's side - I'm not sure how much of a change on the caller's side.

Hm, along that vein, could you do something like:

typedef std::string BinaryOut;
or
using std::string = BinaryOut;

to make the template system behave better?

[Jeffrey Yasskin, 2015/01/13 21:17:21]

Nope. :)

+
 // Creates a new item at |out| from |from|[|index|]. These are used by template
 // specializations of |Get(Optional)ArrayFromList|.
-bool GetItemFromList(const base::ListValue& from, int index, int* out);
-bool GetItemFromList(const base::ListValue& from, int index, bool* out);
-bool GetItemFromList(const base::ListValue& from, int index, double* out);
-bool GetItemFromList(const base::ListValue& from, int index, std::string* out);
 bool GetItemFromList(const base::ListValue& from,
                      int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
+                     int* out);
+bool GetItemFromList(const base::ListValue& from,
+                     int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
+                     bool* out);
+bool GetItemFromList(const base::ListValue& from,
+                     int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
+                     double* out);
+bool GetItemFromList(const base::ListValue& from,
+                     int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
+                     std::string* out);
+bool GetItemFromList(const base::ListValue& from,
+                     int index,
+                     ITEMS_ARE_BINARY,
+                     std::string* out);
+bool GetItemFromList(const base::ListValue& from,
+                     int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
                      linked_ptr<base::Value>* out);
 bool GetItemFromList(const base::ListValue& from,
                      int index,
+                     ITEMS_ARE_NOT_BINARY binary_tag,
                      linked_ptr<base::DictionaryValue>* out);
 
 // This template is used for types generated by tools/json_schema_compiler.
-template<class T>
+template <class T>
 bool GetItemFromList(const base::ListValue& from,
                      int index,
+                     ITEMS_ARE_NOT_BINARY /* binary_tag */,
                      linked_ptr<T>* out) {
   const base::DictionaryValue* dict;
   if (!from.GetDictionary(index, &dict))
@@ -46,13 +68,13 @@ bool GetItemFromList(const base::ListValue& from,
 
 // Populates |out| with |list|. Returns false if there is no list at the
 // specified key or if the list has anything other than |T|.
-template <class T>
+template <class T, class BINARY_TAG>
 bool PopulateArrayFromList(
-    const base::ListValue& list, std::vector<T>* out) {
+    const base::ListValue& list, BINARY_TAG binary_tag, std::vector<T>* out) {
   out->clear();
   T value;
   for (size_t i = 0; i < list.GetSize(); ++i) {
-    if (!GetItemFromList(list, i, &value))
+    if (!GetItemFromList(list, i, binary_tag, &value))
       return false;
     out->push_back(value);
   }
@@ -71,20 +93,20 @@ bool PopulateArrayFromDictionary(
   if (!from.GetListWithoutPathExpansion(name, &list))
     return false;
 
-  return PopulateArrayFromList(*list, out);
+  return PopulateArrayFromList(*list, ITEMS_ARE_NOT_BINARY(), out);
 }
 
 // Creates a new vector containing |list| at |out|. Returns
 // true on success or if there is nothing at the specified key. Returns false
 // if anything other than a list of |T| is at the specified key.
-template <class T>
-bool PopulateOptionalArrayFromList(
-    const base::ListValue& list,
-    scoped_ptr<std::vector<T> >* out) {
+template <class T, class BINARY_TAG>
+bool PopulateOptionalArrayFromList(const base::ListValue& list,
+                                   BINARY_TAG binary_tag,
+                                   scoped_ptr<std::vector<T>>* out) {
   out->reset(new std::vector<T>());
   T value;
   for (size_t i = 0; i < list.GetSize(); ++i) {
-    if (!GetItemFromList(list, i, &value)) {
+    if (!GetItemFromList(list, i, binary_tag, &value)) {
       out->reset();
       return false;
     }
@@ -114,7 +136,7 @@ bool PopulateOptionalArrayFromDictionary(
     list = static_cast<const base::ListValue*>(maybe_list);
   }
 
-  return PopulateOptionalArrayFromList(*list, out);
+  return PopulateOptionalArrayFromList(*list, ITEMS_ARE_NOT_BINARY(), out);
 }
 
 // Appends a Value newly created from |from| to |out|. These used by template