Issue 2005 - Refactor working with strings in InputBuffer and OutputBuffer

src/shared/Communication.h

Index: src/shared/Communication.h
diff --git a/src/shared/Communication.h b/src/shared/Communication.h
index 4caa442dfbfeef7abcb042709cdf84d07366476e..eef73e4c47461bf056b6731568745adf66446b38 100644
--- a/src/shared/Communication.h
+++ b/src/shared/Communication.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 #include <Windows.h>
+#include "Utils.h"
 
 namespace Communication
 {
@@ -38,7 +39,7 @@ namespace Communication
     PROC_COMPARE_VERSIONS
   };
   enum ValueType : uint32_t {
-    TYPE_PROC, TYPE_STRING, TYPE_WSTRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS
+    TYPE_PROC, TYPE_STRING, TYPE_INT64, TYPE_INT32, TYPE_BOOL, TYPE_STRINGS
   };
   typedef uint32_t SizeType;
 
@@ -54,12 +55,25 @@ namespace Communication
       currentType = copy.currentType; 
     }
     InputBuffer& operator>>(ProcType& value) { return Read(value, TYPE_PROC); }
-    InputBuffer& operator>>(std::string& value) { return ReadString(value, TYPE_STRING); }
-    InputBuffer& operator>>(std::wstring& value) { return ReadString(value, TYPE_WSTRING); }
+    InputBuffer& operator>>(std::string& value) { return ReadString(value); }
+    InputBuffer& operator>>(std::wstring& value)
+    {
+      std::string tmpString;
+      operator>>(tmpString);
+      value = ToUtf16String(tmpString);
+      return *this;
+    }
     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); }
     InputBuffer& operator>>(std::vector<std::string>& value) { return ReadStrings(value); }
+    InputBuffer& operator>>(std::vector<std::wstring>& value)
+    {
+      std::vector<std::string> tmpStrings;

[Eric, 2015/02/17 18:15:13]

Ugh. A temporary vector is just ugly and inefficient. Nevertheless, it has
correct behavior and is expedient. We need clean interfaces and automatic
conversion in all the operators first. This operator can be improved later.

+      operator>>(tmpStrings);
+      value = ToUtf16Strings(tmpStrings);
+      return *this;
+    }
     InputBuffer& operator=(const InputBuffer& copy) 
     { 
       hasType = copy.hasType; 
@@ -75,20 +89,16 @@ namespace Communication
 
     void CheckType(ValueType expectedType);
 
-    template<class T>
-    InputBuffer& ReadString(T& value, ValueType expectedType)
+    InputBuffer& ReadString(std::string& value)
     {
-      CheckType(expectedType);
+      CheckType(TYPE_STRING);
 
       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);
+      value.resize(length, '\0');

[Eric, 2015/02/17 18:15:13]

We don't need a fill character, since we're immediately going to overwrite it.

[sergei, 2015/02/18 14:13:06]

I've removed it but it's still used because
void resize(size_type _Newsize)
{
  resize(_Newsize, _Elem());
}

[Eric, 2015/02/18 18:12:47]

That's fine. Nothing about external string initialization in the C++ library is
efficient, but getting rid of the argument reads better.

+      buffer.read(&value[0], sizeof(std::string::value_type) * length);

[Eric, 2015/02/17 18:15:13]

We need two things here.
-- An explicit const_cast to value.data().
-- A comment that we're writing into a buffer declared read-only in the
standard.

[Eric, 2015/02/17 18:15:13]

Separately, we don't need the sizeof expression here. It's not wrong, and the
optimizer will probably make it disappear, but this is std::string, and we don't
need to worry about that sizeof not being 1.

[sergei, 2015/02/18 14:13:06]

done

[sergei, 2015/02/18 14:13:06]

I have just thought about it again and I'm not sure that we need it. We
discussed it already but I cannot find it, could you remind the points again or
point to the discussion, please?
As far as I remember, the arguments were:
1. We cannot be sure that internally the string is stored in the contiguous
buffer.
2. We could use another writable buffer (which is inefficient) because
`std::string::data` returns the pointer to the "read-only" buffer and we are not
supposed to be allowed to write there.

1> It's guaranteed by C++11 standard, otherwise we cannot use any feature of
C++11.
2> Actually, it's a good point to use `&str[0]` instead of `const_cast`. Despite
currently de facto we can use `const_cast`, there might be some reasons to avoid
it. So, why do we need to write suspicious code which violates the
const-correctness?

[sergei, 2015/02/18 14:41:39]

In addition,
Because of
http://en.cppreference.com/w/cpp/string/basic_string/operator_at
1) If pos == size(), the behavior is undefined.
I've added `if (length > 0)` because otherwise it's UB.
I've also added the test, just in case, but, I guess, it won't ever fail.

From
http://en.cppreference.com/w/cpp/string/basic_string/data
"Modifying the character array accessed through data is undefined behavior."
http://en.cppreference.com/w/cpp/string/basic_string/c_str
"Writing to the character array accessed through c_str() is undefined behavior."

[Eric, 2015/02/18 18:12:47]

From my point of view, it is _already_ suspicious code. Using const-cast is a
way of documenting the suspicion.

We're relying on the particular implementation of std::string in MSVC. In other
words, we're using something that only looks like standard. My main concern over
this whole issue is (and has been) that we should not do this silently, that is,
without documentation. It should not be a conventional idiom that passes by
without comment.

I'm beginning to think that the right way to treat this kind of string
initialization is with our own template function. That way we can put all the
documentation in one place and let the use of the function serve as the
point-of-use documentation.

[Eric, 2015/02/18 18:12:47]

From that same page:
    "data()[i] == operator[](i) for every i in [0, size()). (until C++11)"

Given this statement, there is no distinction between &str[0] and str.data(). I
find std.data() a more direct expression, without the legacy burden of C idiom.
You've pointed out that modifying through data() is undefined, which means that
modifying through &str[0] is also undefined; no distinction on that point.

There's a proviso that this is only guaranteed before C++11, but Microsoft
doesn't (much) segregate C++11 and C++03 semantics, so we can expect that the
proviso won't matter for us.

Once we all acknowledge that we are willing to write non-standard code for the
sake of expediency (and even efficiency), it will become clearer what to do
about it.

[sergei, 2015/02/19 17:26:57]

I've found it,
http://codereview.adblockplus.org/5750789393874944/diff/5629499534213120/src/...
.
So, everybody is aware about it and we decided that &str[0] is fine as well as
it does not do anything which is documented as undefined behavour. I would say
it's not suspicious.
We decided that there are no plans to switch to anything else, we are not using
not-working things and, as I understand, MS team does not plan to break
std::string.
I would say it's overengineering here.

[sergei, 2015/02/19 17:26:57]

Don't mix const methods and non-const methods. On that page they are talking
about const methods. As it was mentioned already it's stated on this page that
"Modifying the character array accessed through data is undefined behavior".
It's guaranteed not *before* C++11, it's guaranteed *since* C++11.
What is non-standard and what do we violate here?

       if (buffer.fail())
         throw new std::runtime_error("Unexpected end of input buffer");

[Eric, 2015/02/17 18:15:13]

It wasn't your error, but we should be calling bad() here.

[sergei, 2015/02/18 14:13:06]

Right now I'm not sure about it and it's not relevant to the current changes ,
so it would be better to do in another commit. Could you create the issue
explaining the reasons it's wrong.

[Eric, 2015/02/18 18:12:47]

A separate issue for this is overkill, but very briefly, if an operation sets
the bad bit it will also set the fail bit, but not vice-versa.

See http://www.cplusplus.com/reference/ios/basic_ios/bad/

[sergei, 2015/02/19 17:26:57]

The reason to change it clear, but it's not a single place where we use it, and
it's not relevant to the current change, so I would like to have it in another
commit.

-
-      value.assign(data.get(), length);
       return *this;
     }
 
@@ -138,8 +148,11 @@ namespace Communication
       return buffer.str();
     }
     OutputBuffer& operator<<(ProcType value) { return Write(value, TYPE_PROC); }
-    OutputBuffer& operator<<(const std::string& value) { return WriteString(value, TYPE_STRING); }
-    OutputBuffer& operator<<(const std::wstring& value) { return WriteString(value, TYPE_WSTRING); }
+    OutputBuffer& operator<<(const std::string& value) { return WriteString(value); }
+    OutputBuffer& operator<<(const std::wstring& value)
+    {
+      return operator<<(ToUtf8String(value));
+    }
     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); }
@@ -150,15 +163,13 @@ namespace Communication
     // Disallow copying
     const OutputBuffer& operator=(const OutputBuffer&);
 
-    template<class T>
-    OutputBuffer& WriteString(const T& value, ValueType type)
+    OutputBuffer& WriteString(const std::string& value)
     {
-      WriteBinary(type);
+      WriteBinary(TYPE_STRING);
 
       SizeType length = static_cast<SizeType>(value.size());
       WriteBinary(length);
-
-      buffer.write(reinterpret_cast<const char*>(value.c_str()), sizeof(T::value_type) * length);
+      buffer.write(value.c_str(), sizeof(std::string::value_type) * length);

[Eric, 2015/02/17 18:15:13]

See sizeof comment above.

[sergei, 2015/02/18 14:13:06]

done.

       if (buffer.fail())
         throw new std::runtime_error("Unexpected error writing to output buffer");