Cleanup: Some simplifications in json_schema_compiler.

tools/json_schema_compiler/util.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef TOOLS_JSON_SCHEMA_COMPILER_UTIL_H__
 #define TOOLS_JSON_SCHEMA_COMPILER_UTIL_H__
 
 #include <string>
 #include <vector>
 
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
 
 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))
     return false;
   scoped_ptr<T> obj(new T());
   if (!T::Populate(*dict, obj.get()))
     return false;
   *out = linked_ptr<T>(obj.release());
   return true;
 }
 
 // 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);
   }
 
   return true;
 }
 
 // Populates |out| with |from|.|name|. Returns false if there is no list at
 // the specified key or if the list has anything other than |T|.
 template <class T>
 bool PopulateArrayFromDictionary(
     const base::DictionaryValue& from,
     const std::string& name,
     std::vector<T>* out) {
   const base::ListValue* list = NULL;
   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;
     }
     (*out)->push_back(value);
   }
 
   return true;
 }
 
 // Creates a new vector containing |from|.|name| 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 PopulateOptionalArrayFromDictionary(
     const base::DictionaryValue& from,
     const std::string& name,
     scoped_ptr<std::vector<T> >* out) {
   const base::ListValue* list = NULL;
   {
     const base::Value* maybe_list = NULL;
     // Since |name| is optional, its absence is acceptable. However, anything
     // other than a ListValue is not.
     if (!from.GetWithoutPathExpansion(name, &maybe_list))
       return true;
     if (!maybe_list->IsType(base::Value::TYPE_LIST))
       return false;
     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
 // specializations of |Set(Optional)ArrayToList|.
 void AddItemToList(const int from, base::ListValue* out);
 void AddItemToList(const bool from, base::ListValue* out);
 void AddItemToList(const double from, base::ListValue* out);
 void AddItemToList(const std::string& from, base::ListValue* out);
 void AddItemToList(const linked_ptr<base::Value>& from,
                    base::ListValue* out);
@@ skipping 44 matching lines @@
     return CreateValueFromArray(*from);
   return scoped_ptr<base::Value>();
 }
 
 std::string ValueTypeToString(base::Value::Type type);
 
 }  // namespace util
 }  // namespace json_schema_compiler
 
 #endif // TOOLS_JSON_SCHEMA_COMPILER_UTIL_H__