* Change C++ serialization/deserialization to not be move-only operations (with the except of |Ha…

mojo/public/cpp/bindings/lib/array_serialization.h

Index: mojo/public/cpp/bindings/lib/array_serialization.h
diff --git a/mojo/public/cpp/bindings/lib/array_serialization.h b/mojo/public/cpp/bindings/lib/array_serialization.h
index 9d0e7bb438d0c6137f712c0c984cf733f78fa587..e59788a01be871ebd1f48bf87c8d04cf483b337d 100644
--- a/mojo/public/cpp/bindings/lib/array_serialization.h
+++ b/mojo/public/cpp/bindings/lib/array_serialization.h
@@ -6,12 +6,13 @@
 #define MOJO_PUBLIC_CPP_BINDINGS_LIB_ARRAY_SERIALIZATION_H_
 
 #include <string.h>  // For |memcpy()|.
-
 #include <vector>
 
 #include "mojo/public/c/system/macros.h"
 #include "mojo/public/cpp/bindings/lib/array_internal.h"
-#include "mojo/public/cpp/bindings/lib/map_serialization.h"
+#include "mojo/public/cpp/bindings/lib/bindings_internal.h"
+#include "mojo/public/cpp/bindings/lib/iterator_util.h"
+#include "mojo/public/cpp/bindings/lib/map_data_internal.h"
 #include "mojo/public/cpp/bindings/lib/string_serialization.h"
 #include "mojo/public/cpp/bindings/lib/template_util.h"
 #include "mojo/public/cpp/bindings/lib/validation_errors.h"
@@ -23,7 +24,7 @@ inline size_t GetSerializedSize_(const Array<E>& input);
 
 template <typename E, typename F>
 inline void SerializeArray_(
-    Array<E> input,
+    Array<E>* input,
     internal::Buffer* buf,
     internal::Array_Data<F>** output,
     const internal::ArrayValidateParams* validate_params);
@@ -31,8 +32,41 @@ inline void SerializeArray_(
 template <typename E, typename F>
 inline void Deserialize_(internal::Array_Data<F>* data, Array<E>* output);
 
+// To avoid a circular dependency on map_serialization.h, we forward declare:

[viettrungluu, 2015/09/23 18:15:55]

Maybe we should have a map_serialization_forward.h?

(Possibly this applies to other things which we have forward declarations for.)

Side point: I wonder if we should have the trailing _, for things that are
"exposed" (and if they're not exposed, maybe they should be in the |internal|
namespace).

[vardhan, 2015/09/23 22:07:00]

I think SerializeMap_ should be internal (not available to mojo apps to use), 
because SerializeMap_ is not enough to serialize a map and write it to disk --
you also need to encode handles and pointers.

But  if we had a public version and our internal version, I think having a
mojo::SerializeMap() version and an internal mojo::internal::SerializeMap() is a
little unambiguous, so I was thinking we should keep the trailing '_'?

LMK what you think, I'll do a follow up CL for this.

[vardhan, 2015/09/23 22:12:16]

Sorry, I meant its a little ambigious*

[viettrungluu, 2015/09/23 23:20:24]

Probably it should be moved to internal:: (without the trailing _).

(The only reason for trailing _s, is when we generate stuff in someone else's
namespace, or similarly generating members/methods in a class whose
members/methods are controlled by someone else.)

[vardhan, 2015/09/25 19:33:05]

Another thought is that I would have to leave Deserialize_ (with the
underscore), which creates an underscore inconsistency..

Would you mind if i left this renaming to another CL?

[viettrungluu, 2015/09/25 20:08:08]

OK

+template <typename Key, typename Value>
+class Map;
+
+template <typename MapKey,
+          typename MapValue,
+          typename DataKey,
+          typename DataValue>
+void SerializeMap_(Map<MapKey, MapValue>* input,
+                   internal::Buffer* buf,
+                   internal::Map_Data<DataKey, DataValue>** output,
+                   const internal::ArrayValidateParams* value_validate_params);
+template <typename MapKey, typename MapValue>
+size_t GetSerializedSize_(const Map<MapKey, MapValue>& input);
+
+template <typename MapKey,
+          typename MapValue,
+          typename DataKey,
+          typename DataValue>
+void Deserialize_(internal::Map_Data<DataKey, DataValue>* input,
+                  Map<MapKey, MapValue>* output);
+
 namespace internal {
 
+// ArraySerializer template contains static methods for serializing |Array|s of

[viettrungluu, 2015/09/23 18:15:55]

*The* ArraySerializer template ...

[vardhan, 2015/09/23 22:07:00]

Done.

+// various types.  These methods include:
+//   * size_t GetSerializedSize(..)
+//       Computes the size of the serialized version of the |Array|.
+//   * void SerializeElementsWithIterator(..)
+//       Takes an iterator and |num_elements| representing an |Array| (or
+//       possibly a |Map|), and serializes it.
+//   * void SerializeElements(..)
+//       Takes an |Array| directly and serializes it.  This usually calls into
+//       SerializeElementsWithIterator(), except for an |Array| of PODs which
+//       will directly memcpy the PODs as an optimization.
 template <typename E,
           typename F,
           bool is_union =
@@ -47,7 +81,26 @@ struct ArraySerializer<E, F, false> {
     return sizeof(Array_Data<F>) + Align(input.size() * sizeof(E));
   }
 
-  static void SerializeElements(Array<E> input,
+  template <typename Iterator>
+  static void SerializeElementsWithIterator(
+      Iterator it,
+      size_t num_elements,
+      Buffer* buf,
+      Array_Data<F>* output,
+      const ArrayValidateParams* validate_params) {
+    MOJO_DCHECK(!validate_params->element_is_nullable)
+        << "Primitive type should be non-nullable";
+    MOJO_DCHECK(!validate_params->element_validate_params)
+        << "Primitive type should not have array validate params";
+    if (num_elements) {
+      for (size_t i = 0; i < num_elements; ++i, ++it) {
+        output->at(i) = *it;
+      }
+    }
+  }
+
+  // We can optimize serializing PODs by |memcpy|ing directly.
+  static void SerializeElements(Array<E>* input,
                                 Buffer* buf,
                                 Array_Data<F>* output,
                                 const ArrayValidateParams* validate_params) {
@@ -55,10 +108,12 @@ struct ArraySerializer<E, F, false> {
         << "Primitive type should be non-nullable";
     MOJO_DCHECK(!validate_params->element_validate_params)
         << "Primitive type should not have array validate params";
-
-    if (input.size())
-      memcpy(output->storage(), &input.storage()[0], input.size() * sizeof(E));
+    if (input->size()) {
+      memcpy(output->storage(), &input->storage()[0],
+             input->size() * sizeof(E));
+    }
   }
+
   static void DeserializeElements(Array_Data<F>* input, Array<E>* output) {
     std::vector<E> result(input->size());
     if (input->size())
@@ -74,19 +129,32 @@ struct ArraySerializer<bool, bool, false> {
     return sizeof(Array_Data<bool>) + Align((input.size() + 7) / 8);
   }
 
-  static void SerializeElements(Array<bool> input,
-                                Buffer* buf,
-                                Array_Data<bool>* output,
-                                const ArrayValidateParams* validate_params) {
+  template <typename Iterator>
+  static void SerializeElementsWithIterator(
+      Iterator it,
+      size_t num_elements,
+      Buffer* buf,
+      Array_Data<bool>* output,
+      const ArrayValidateParams* validate_params) {
     MOJO_DCHECK(!validate_params->element_is_nullable)
         << "Primitive type should be non-nullable";
     MOJO_DCHECK(!validate_params->element_validate_params)
         << "Primitive type should not have array validate params";
 
     // TODO(darin): Can this be a memcpy somehow instead of a bit-by-bit copy?
-    for (size_t i = 0; i < input.size(); ++i)
-      output->at(i) = input[i];
+    for (size_t i = 0; i < num_elements; ++i, ++it) {
+      output->at(i) = *it;
+    }
+  }
+
+  static void SerializeElements(Array<bool>* input,
+                                Buffer* buf,
+                                Array_Data<bool>* output,
+                                const ArrayValidateParams* validate_params) {
+    SerializeElementsWithIterator(input->begin(), input->size(), buf, output,
+                                  validate_params);
   }
+
   static void DeserializeElements(Array_Data<bool>* input,
                                   Array<bool>* output) {
     Array<bool> result(input->size());
@@ -104,23 +172,36 @@ struct ArraySerializer<ScopedHandleBase<H>, H, false> {
     return sizeof(Array_Data<H>) + Align(input.size() * sizeof(H));
   }
 
-  static void SerializeElements(Array<ScopedHandleBase<H>> input,
-                                Buffer* buf,
-                                Array_Data<H>* output,
-                                const ArrayValidateParams* validate_params) {
+  template <typename Iterator>
+  static void SerializeElementsWithIterator(
+      Iterator it,
+      size_t num_elements,
+      Buffer* buf,
+      Array_Data<H>* output,
+      const ArrayValidateParams* validate_params) {
     MOJO_DCHECK(!validate_params->element_validate_params)
         << "Handle type should not have array validate params";
 
-    for (size_t i = 0; i < input.size(); ++i) {
-      output->at(i) = input[i].release();  // Transfer ownership of the handle.
+    for (size_t i = 0; i < num_elements; ++i, ++it) {
+      // Transfer ownership of the handle.
+      output->at(i) = (*it).release();
       MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(
           !validate_params->element_is_nullable && !output->at(i).is_valid(),
           VALIDATION_ERROR_UNEXPECTED_INVALID_HANDLE,
           MakeMessageWithArrayIndex(
-              "invalid handle in array expecting valid handles", input.size(),
+              "invalid handle in array expecting valid handles", num_elements,
               i));
     }
   }
+
+  static void SerializeElements(Array<ScopedHandleBase<H>>* input,

[viettrungluu, 2015/09/23 18:15:55]

Do you really need this? (Not having it would also allow you to keep the
"SerializeElements" name for the one that takes an iterator.)

[vardhan, 2015/09/23 22:06:59]

The only reason we have both SerializeElements and SerializeIterableElements is
so that SerializeElements can memcpy() Array<POD type> (minus bool) since it's
just a vector instead of having to work with an iterator (see line 114)

[viettrungluu, 2015/09/23 23:20:24]

Is it possible to do this via some appropriate specialization? All the extra
boilerplate is really unfortunate.

[vardhan, 2015/09/25 19:33:05]

Done :(

+                                Buffer* buf,
+                                Array_Data<H>* output,
+                                const ArrayValidateParams* validate_params) {
+    SerializeElementsWithIterator(input->begin(), input->size(), buf, output,
+                                  validate_params);
+  }
+
   static void DeserializeElements(Array_Data<H>* input,
                                   Array<ScopedHandleBase<H>>* output) {
     Array<ScopedHandleBase<H>> result(input->size());
@@ -130,8 +211,84 @@ struct ArraySerializer<ScopedHandleBase<H>, H, false> {
   }
 };
 
-// This template must only apply to pointer mojo entity (structs and arrays).
-// This is done by ensuring that WrapperTraits<S>::DataType is a pointer.
+// SerializeCaller template is used by |ArraySerializer| to dispatch a serialize
+// call on a non-POD type.  This template is defined outside |ArraySerializer|
+// since you cannot specialize a struct within a class definition.

[viettrungluu, 2015/09/23 18:15:55]

Are you allowed to specialize static member functions?

[vardhan, 2015/09/23 22:06:59]

Yeah, but explicitly specialized definitions of a member function needs to live
outside of the struct definition still.  Most of this trouble is because I'm
fully specializing for String (ie SerializeCaller<String>)

[viettrungluu, 2015/09/23 23:20:24]

Well, the definition would be declared outside, but at least you wouldn't be
dumping all of this inside the global namespace?

(And not having the extra struct would also reduce verbiage too.)

[vardhan, 2015/09/25 19:33:05]

Done.

+template <typename T>
+struct SerializeCaller {
+  static void Run(T* input,
+                  Buffer* buf,
+                  typename WrapperTraits<T>::DataType* output,
+                  const ArrayValidateParams* validate_params) {
+    MOJO_DCHECK(!validate_params)
+        << "Struct type should not have array validate params";
+    Serialize_(UnwrapStructPtr<T>::value(*input), buf, output);
+  }
+};
+
+template <>
+struct SerializeCaller<String> {
+  static void Run(const String* input,
+                  Buffer* buf,
+                  String_Data** output,
+                  const ArrayValidateParams* validate_params) {
+    MOJO_DCHECK(validate_params && !validate_params->element_validate_params &&
+                !validate_params->element_is_nullable &&
+                validate_params->expected_num_elements == 0)
+        << "String type has unexpected array validate params";
+    SerializeString_(*input, buf, output);
+  }
+};
+
+template <typename T>
+struct SerializeCaller<Array<T>> {
+  static void Run(Array<T>* input,
+                  Buffer* buf,
+                  typename Array<T>::Data_** output,
+                  const ArrayValidateParams* validate_params) {
+    SerializeArray_(input, buf, output, validate_params);
+  }
+};
+
+template <typename Key, typename Value>
+struct SerializeCaller<Map<Key, Value>> {
+  static void Run(Map<Key, Value>* input,
+                  Buffer* buf,
+                  typename Map<Key, Value>::Data_** output,
+                  const ArrayValidateParams* validate_params) {
+    SerializeMap_(input, buf, output, validate_params);
+  }
+};
+
+template <typename T,
+          bool IsStructPtr = IsSpecializationOf<StructPtr, T>::value ||
+                             IsSpecializationOf<InlinedStructPtr, T>::value>
+struct DeserializeCaller;
+
+template <typename T>
+struct DeserializeCaller<T, false> {
+  static void Run(typename WrapperTraits<T>::DataType input, T* output) {
+    Deserialize_(input, output);
+  }
+};
+
+template <typename T>
+struct DeserializeCaller<T, true> {
+  static void Run(typename WrapperTraits<T>::DataType input, T* output) {
+    // Since Deserialize_ takes in a |Struct*| (not |StructPtr|), we need to
+    // initialize the |StructPtr| here before deserializing into its underlying
+    // data.
+    // TODO(vardhan):  Either all containers, or just Deserialize_(), should
+    // support taking in an allocator.
+    using UnwrapedStructType = typename RemoveStructPtr<T>::type;
+    *output = UnwrapedStructType::New();
+    Deserialize_(input, output->get());
+  }
+};
+
+// This template must only apply to pointer mojo entity (structs, arrays,
+// strings).  This is done by ensuring that WrapperTraits<S>::DataType is a
+// pointer.
 template <typename S>
 struct ArraySerializer<
     S,
@@ -144,68 +301,46 @@ struct ArraySerializer<
     size_t size = sizeof(Array_Data<S_Data*>) +
                   input.size() * sizeof(StructPointer<S_Data>);
     for (size_t i = 0; i < input.size(); ++i)
-      size += GetSerializedSize_(input[i]);
+      size += GetSerializedSize_(*(UnwrapConstStructPtr<S>::value(input[i])));
     return size;
   }
 
-  static void SerializeElements(Array<S> input,
-                                Buffer* buf,
-                                Array_Data<S_Data*>* output,
-                                const ArrayValidateParams* validate_params) {
-    for (size_t i = 0; i < input.size(); ++i) {
+  template <typename Iterator>
+  static void SerializeElementsWithIterator(
+      Iterator it,
+      size_t num_elements,
+      Buffer* buf,
+      Array_Data<S_Data*>* output,
+      const ArrayValidateParams* validate_params) {
+    for (size_t i = 0; i < num_elements; ++i, ++it) {
       S_Data* element;
-      SerializeCaller<S>::Run(input[i].Pass(), buf, &element,
+      SerializeCaller<S>::Run(&(*it), buf, &element,
                               validate_params->element_validate_params);
       output->at(i) = element;
       MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(
           !validate_params->element_is_nullable && !element,
           VALIDATION_ERROR_UNEXPECTED_NULL_POINTER,
           MakeMessageWithArrayIndex("null in array expecting valid pointers",
-                                    input.size(), i));
+                                    num_elements, i));
     }
   }
+
+  static void SerializeElements(Array<S>* input,
+                                Buffer* buf,
+                                Array_Data<S_Data*>* output,
+                                const ArrayValidateParams* validate_params) {
+    SerializeElementsWithIterator(input->begin(), input->size(), buf, output,
+                                  validate_params);
+  }
+
   static void DeserializeElements(Array_Data<S_Data*>* input,
                                   Array<S>* output) {
     Array<S> result(input->size());
     for (size_t i = 0; i < input->size(); ++i) {
-      Deserialize_(input->at(i), &result[i]);
+      DeserializeCaller<S>::Run(input->at(i), &result[i]);
     }
     output->Swap(&result);
   }
-
- private:
-  template <typename T>
-  struct SerializeCaller {
-    static void Run(T input,
-                    Buffer* buf,
-                    typename WrapperTraits<T>::DataType* output,
-                    const ArrayValidateParams* validate_params) {
-      MOJO_DCHECK(!validate_params)
-          << "Struct type should not have array validate params";
-
-      Serialize_(input.Pass(), buf, output);
-    }
-  };
-
-  template <typename T>
-  struct SerializeCaller<Array<T>> {
-    static void Run(Array<T> input,
-                    Buffer* buf,
-                    typename Array<T>::Data_** output,
-                    const ArrayValidateParams* validate_params) {
-      SerializeArray_(input.Pass(), buf, output, validate_params);
-    }
-  };
-
-  template <typename T, typename U>
-  struct SerializeCaller<Map<T, U>> {
-    static void Run(Map<T, U> input,
-                    Buffer* buf,
-                    typename Map<T, U>::Data_** output,
-                    const ArrayValidateParams* validate_params) {
-      SerializeMap_(input.Pass(), buf, output, validate_params);
-    }
-  };
 };
 
 // Handles serialization and deserialization of arrays of unions.
@@ -221,68 +356,42 @@ struct ArraySerializer<U, U_Data, true> {
     return size;
   }
 
-  static void SerializeElements(Array<U> input,
-                                Buffer* buf,
-                                Array_Data<U_Data>* output,
-                                const ArrayValidateParams* validate_params) {
-    for (size_t i = 0; i < input.size(); ++i) {
+  template <typename Iterator>
+  static void SerializeElementsWithIterator(
+      Iterator it,
+      size_t num_elements,
+      Buffer* buf,
+      Array_Data<U_Data>* output,
+      const ArrayValidateParams* validate_params) {
+    for (size_t i = 0; i < num_elements; ++i, ++it) {
       U_Data* result = output->storage() + i;
-      SerializeUnion_(input[i].Pass(), buf, &result, true);
+      SerializeUnion_((*it).get(), buf, &result, true);
       MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(
           !validate_params->element_is_nullable && output->at(i).is_null(),
           VALIDATION_ERROR_UNEXPECTED_NULL_POINTER,
           MakeMessageWithArrayIndex("null in array expecting valid unions",
-                                    input.size(), i));
+                                    num_elements, i));
     }
   }
 
-  static void DeserializeElements(Array_Data<U_Data>* input, Array<U>* output) {
-    Array<U> result(input->size());
-    for (size_t i = 0; i < input->size(); ++i) {
-      Deserialize_(&input->at(i), &result[i]);
-    }
-    output->Swap(&result);
-  }
-};
-
-// Handles serialization and deserialization of arrays of strings.
-template <>
-struct ArraySerializer<String, String_Data*> {
-  static size_t GetSerializedSize(const Array<String>& input) {
-    size_t size =
-        sizeof(Array_Data<String_Data*>) + input.size() * sizeof(StringPointer);
-    for (size_t i = 0; i < input.size(); ++i)
-      size += GetSerializedSize_(input[i]);
-    return size;
-  }
-
-  static void SerializeElements(Array<String> input,
+  static void SerializeElements(Array<U>* input,
                                 Buffer* buf,
-                                Array_Data<String_Data*>* output,
+                                Array_Data<U_Data>* output,
                                 const ArrayValidateParams* validate_params) {
-    MOJO_DCHECK(
-        validate_params->element_validate_params &&
-        !validate_params->element_validate_params->element_validate_params &&
-        !validate_params->element_validate_params->element_is_nullable &&
-        validate_params->element_validate_params->expected_num_elements == 0)
-        << "String type has unexpected array validate params";
+    SerializeElementsWithIterator(input->begin(), input->size(), buf, output,
+                                  validate_params);
+  }
 
-    for (size_t i = 0; i < input.size(); ++i) {
-      String_Data* element;
-      Serialize_(input[i], buf, &element);
-      output->at(i) = element;
-      MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(
-          !validate_params->element_is_nullable && !element,
-          VALIDATION_ERROR_UNEXPECTED_NULL_POINTER,
-          MakeMessageWithArrayIndex("null in array expecting valid strings",
-                                    input.size(), i));
+  static void DeserializeElements(Array_Data<U_Data>* input, Array<U>* output) {
+    Array<U> result(input->size());
+    for (size_t i = 0; i < input->size(); ++i) {
+      auto& elem = input->at(i);
+      if (!elem.is_null()) {
+        using UnwrapedUnionType = typename RemoveStructPtr<U>::type;
+        result[i] = UnwrapedUnionType::New();
+        Deserialize_(&elem, result[i].get());
+      }
     }
-  }
-  static void DeserializeElements(Array_Data<String_Data*>* input,
-                                  Array<String>* output) {
-    Array<String> result(input->size());
-    for (size_t i = 0; i < input->size(); ++i)
-      Deserialize_(input->at(i), &result[i]);
     output->Swap(&result);
   }
 };
@@ -299,24 +408,24 @@ inline size_t GetSerializedSize_(const Array<E>& input) {
 
 template <typename E, typename F>
 inline void SerializeArray_(
-    Array<E> input,
+    Array<E>* input,
     internal::Buffer* buf,
     internal::Array_Data<F>** output,
     const internal::ArrayValidateParams* validate_params) {
-  if (input) {
+  if (input && *input) {
     MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(
         validate_params->expected_num_elements != 0 &&
-            input.size() != validate_params->expected_num_elements,
+            input->size() != validate_params->expected_num_elements,
         internal::VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER,
         internal::MakeMessageWithExpectedArraySize(
-            "fixed-size array has wrong number of elements", input.size(),
+            "fixed-size array has wrong number of elements", input->size(),
             validate_params->expected_num_elements));
 
     internal::Array_Data<F>* result =
-        internal::Array_Data<F>::New(input.size(), buf);
+        internal::Array_Data<F>::New(input->size(), buf);
     if (result) {
-      internal::ArraySerializer<E, F>::SerializeElements(
-          internal::Forward(input), buf, result, validate_params);
+      internal::ArraySerializer<E, F>::SerializeElements(input, buf, result,
+                                                         validate_params);
     }
     *output = result;
   } else {