Implement better marshaling

src/shared/Communication.h

 #ifndef COMMUNICATION_H
 #define COMMUNICATION_H
 
+#include <memory>
 #include <sstream>
+#include <stdexcept>
 #include <stdint.h>
 #include <string>
 #include <vector>
 #include <Windows.h>
 
 namespace Communication
 {
   extern const std::wstring pipeName;
   const int bufferSize = 1024;
 
-  enum ValueType {TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL};
+  enum {TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL};
+  typedef int32_t ValueType;
+  typedef uint32_t SizeType;
 
   class InputBuffer
   {
   public:
-    InputBuffer(const std::string& data) : buffer(data) {}
+    InputBuffer(const std::string& data) : buffer(data), hasType(false) {}
     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;
+    ValueType currentType;
+    bool hasType;
+
+    void CheckType(ValueType expectedType)
+    {
+      if (!hasType)
+        ReadBinary(currentType);
+
+      if (currentType != expectedType)
+      {
+        // Make sure we don't attempt to read the type again
+        hasType = true;
+        throw new std::runtime_error("Unexpected type found in input buffer");
+      }
+      else
+        hasType = false;
+    }
 
     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");
+      CheckType(expectedType);
 
-      uint32_t length;

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

Would prefer a typedef for this.

+      SizeType length;
       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");
-
+      CheckType(expectedType);
       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;
 
+    // Disallow copying
+    const OutputBuffer& operator=(const OutputBuffer&);
+
     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.

+    OutputBuffer& WriteString(const T& value, ValueType type)
     {
       WriteBinary(type);
 
-      uint32_t length = value.size();
+      SizeType 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.

+    OutputBuffer& Write(const T value, ValueType type)

[Felix Dahlke, 2013/06/03 12:23:54]

Shouldn't value be passed by reference?

[Wladimir Palant, 2013/06/03 12:25:55]

Given that this function is meant for primitive types only (ints and bools), do
you think that this would be wise?

[Felix Dahlke, 2013/06/04 06:41:38]

Right, didn't think of that. Won't hurt, but it won't hurt right now either.

     {
       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);
+  class PipeConnectionError : public std::runtime_error
+  {
+  public:
+    PipeConnectionError();
+  };
+
+  class Pipe
+  {
+  public:
+    enum Mode {MODE_CREATE, MODE_CONNECT};
+
+    Pipe(const std::wstring& name, Mode mode);
+    ~Pipe();
+
+    InputBuffer ReadMessage();
+    void WriteMessage(OutputBuffer& message);
+
+  protected:
+    HANDLE pipe;
+  };
 }
 
 #endif