Implement better marshaling

src/shared/Communication.h

 #ifndef COMMUNICATION_H
 #define COMMUNICATION_H
 
+#include <sstream>
+#include <stdint.h>
 #include <string>
 #include <vector>
 #include <Windows.h>
 
 namespace Communication
 {
   extern const std::wstring pipeName;
   const int bufferSize = 1024;
 
-  std::string MarshalStrings(const std::vector<std::string>& strings);
-  std::vector<std::string> UnmarshalStrings(const std::string& message);
-  std::string ReadMessage(HANDLE pipe);
-  void WriteMessage(HANDLE pipe, const std::string& message);
+  enum ValueType {TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL};
+
+  class InputBuffer
+  {
+  public:
+    InputBuffer(const std::string& data) : buffer(data) {}
+    InputBuffer& operator>>(std::string& value) { return ReadString(value, TYPE_STRING); }
+    InputBuffer& operator>>(std::wstring& value) { return ReadString(value, TYPE_WSTRING); }
+    InputBuffer& operator>>(int64_t& value) { return Read(value, TYPE_INT64); }
+    InputBuffer& operator>>(int32_t& value) { return Read(value, TYPE_INT32); }
+    InputBuffer& operator>>(bool& value) { return Read(value, TYPE_BOOL); }
+  private:
+    std::istringstream buffer;
+
+    template<class T>
+    InputBuffer& ReadString(T& value, ValueType expectedType)

[Felix Dahlke, 2013/06/03 11:25:41]

I'd just call this "ReadArray", since it would work with int arrays just as well
as with char arrays.

[Wladimir Palant, 2013/06/03 11:53:00]

Nope - this is expecting std::base_string rather than an array. We want an
explicit string length.

+    {
+      int32_t type;

[Felix Dahlke, 2013/06/03 11:25:41]

Should be ValueType, not int32_t I guess?

[Wladimir Palant, 2013/06/03 11:53:00]

I'm not sure whether the internal representation of ValueType will still be the
same for x86 and x64 clients (the "server" is always x86). So I'm casting to
int32_t to ensure that we are always sending the same type.

[Felix Dahlke, 2013/06/03 12:02:09]

Fair enough. But I'd want a typedef then :)

+      ReadBinary(type);
+      if (type != expectedType)
+        throw new std::runtime_error("Unexpected type found in input buffer");
+
+      uint32_t length;

[Felix Dahlke, 2013/06/03 11:25:41]

Would prefer a typedef for this.

+      ReadBinary(length);
+
+      std::auto_ptr<T::value_type> data(new T::value_type[length]);
+      buffer.read(reinterpret_cast<char*>(data.get()), sizeof(T::value_type) * length);
+      if (buffer.fail())
+        throw new std::runtime_error("Unexpected end of input buffer");
+
+      value.assign(data.get(), length);

[Felix Dahlke, 2013/06/03 11:25:41]

Based on some quick searching, only std::basic_string seems to have an assign
member function with that signature (it's (length, data) for vector e.g.). If
that's the case and we can just handle strings here, there's no need to use have
a template. In that case I wouldn't want to call it ReadArray, of course.

[Wladimir Palant, 2013/06/03 11:53:00]

See above - this function is used both for std::string and std::wstring.

[Felix Dahlke, 2013/06/03 12:02:09]

I thought basic_string was a base class, but it's a template. So this is fine.

+      return *this;
+    }
+
+    template<class T>
+    InputBuffer& Read(T& value, ValueType expectedType)
+    {
+      int32_t type;

[Felix Dahlke, 2013/06/03 11:25:41]

Like above, I think this should be ValueType

+      ReadBinary(type);
+      if (type != expectedType)
+        throw new std::runtime_error("Unexpected type found in input buffer");
+
+      ReadBinary(value);
+      return *this;
+    }
+
+    template<class T>
+    void ReadBinary(T& value)
+    {
+      buffer.read(reinterpret_cast<char*>(&value), sizeof(T));
+      if (buffer.fail())
+        throw new std::runtime_error("Unexpected end of input buffer");
+    }
+  };
+
+  class OutputBuffer
+  {
+  public:
+    OutputBuffer() {}
+
+    // Explicit copy constructor to allow returning OutputBuffer by value

[Felix Dahlke, 2013/06/03 11:25:41]

I find this kind of obvious :P

[Wladimir Palant, 2013/06/03 11:53:00]

It wasn't to me :)

+    OutputBuffer(const OutputBuffer& copy) : buffer(copy.buffer.str()) {}

[Felix Dahlke, 2013/06/03 11:25:41]

We should implement the copy assign operator as well then.

[Wladimir Palant, 2013/06/03 11:53:00]

I think I'm fine with copy assignment failing - I merely implemented this
because we need to return OutputBuffer "by value" occasionally. And I'm not even
sure that this is a good idea.

[Felix Dahlke, 2013/06/03 12:02:09]

Assignment won't fail if the copy assignment operator is implicitly generated,
and I think it will be in this case. See the Rule of Three (Rule of Two here, as
the members use RAII).

+
+    std::string Get()
+    {
+      return buffer.str();
+    }
+    OutputBuffer& operator<<(const std::string& value) { return WriteString(value, TYPE_STRING); }
+    OutputBuffer& operator<<(const std::wstring& value) { return WriteString(value, TYPE_WSTRING); }
+    OutputBuffer& operator<<(int64_t value) { return Write(value, TYPE_INT64); }
+    OutputBuffer& operator<<(int32_t value) { return Write(value, TYPE_INT32); }
+    OutputBuffer& operator<<(bool value) { return Write(value, TYPE_BOOL); }
+  private:
+    std::ostringstream buffer;
+
+    template<class T>
+    OutputBuffer& WriteString(const T& value, int32_t type)

[Felix Dahlke, 2013/06/03 11:25:41]

Should be ValueType instead of int32_t I guess.

+    {
+      WriteBinary(type);
+
+      uint32_t length = value.size();
+      WriteBinary(length);
+
+      buffer.write(reinterpret_cast<const char*>(value.c_str()), sizeof(T::value_type) * length);

[Felix Dahlke, 2013/06/03 11:25:41]

AFAIK, only std::basic_string has c_str(). So no need for the template then.

[Wladimir Palant, 2013/06/03 11:53:00]

See above - both std::string and std::wstring are being used here.

+      if (buffer.fail())
+        throw new std::runtime_error("Unexpected error writing to output buffer");
+
+      return *this;
+    }
+
+    template<class T>
+    OutputBuffer& Write(const T value, int32_t type)

[Felix Dahlke, 2013/06/03 11:25:41]

Should be ValueType instead of int32_t I think. Also, a const T& would probably
be better for the value.

+    {
+      WriteBinary(type);
+      WriteBinary(value);
+      return *this;
+    }
+
+    template<class T>
+    void WriteBinary(const T& value)
+    {
+      buffer.write(reinterpret_cast<const char*>(&value), sizeof(T));
+      if (buffer.fail())
+        throw new std::runtime_error("Unexpected error writing to output buffer");
+    }
+  };
+
+  InputBuffer ReadMessage(HANDLE pipe);
+  void WriteMessage(HANDLE pipe, OutputBuffer& message);
 }
 
 #endif