Issue 10824027: Implement better marshaling (Closed)
|
Created:
May 31, 2013, 8:26 a.m. by Wladimir Palant Modified:
June 19, 2013, 5:37 p.m. Visibility:
Public. |
DescriptionImplement better marshaling
Patch Set 1 #
Total comments: 19
Patch Set 2 : #
Total comments: 3
MessagesTotal messages: 9
On 2013/05/31 08:27:02, Wladimir Palant wrote: LGTM
This is similar to what I had in mind, although my approach didn't include types. Just pairs of length and data, preceeded by a single integer giving the number of elements. But this is probably better because it's type safe :) http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:30: InputBuffer& ReadString(T& value, ValueType expectedType) I'd just call this "ReadArray", since it would work with int arrays just as well as with char arrays. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:32: int32_t type; Should be ValueType, not int32_t I guess? http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:37: uint32_t length; Would prefer a typedef for this. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:45: value.assign(data.get(), length); 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. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:52: int32_t type; Like above, I think this should be ValueType http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:75: // Explicit copy constructor to allow returning OutputBuffer by value I find this kind of obvious :P http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:76: OutputBuffer(const OutputBuffer& copy) : buffer(copy.buffer.str()) {} We should implement the copy assign operator as well then. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:91: OutputBuffer& WriteString(const T& value, int32_t type) Should be ValueType instead of int32_t I guess. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:98: buffer.write(reinterpret_cast<const char*>(value.c_str()), sizeof(T::value_type) * length); AFAIK, only std::basic_string has c_str(). So no need for the template then. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:106: OutputBuffer& Write(const T value, int32_t type) Should be ValueType instead of int32_t I think. Also, a const T& would probably be better for the value.
http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:30: InputBuffer& ReadString(T& value, ValueType expectedType) On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > I'd just call this "ReadArray", since it would work with int arrays just as well > as with char arrays. Nope - this is expecting std::base_string rather than an array. We want an explicit string length. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:32: int32_t type; On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > Should be ValueType, not int32_t I guess? 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. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:45: value.assign(data.get(), length); See above - this function is used both for std::string and std::wstring. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:75: // Explicit copy constructor to allow returning OutputBuffer by value It wasn't to me :) http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:76: OutputBuffer(const OutputBuffer& copy) : buffer(copy.buffer.str()) {} On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > We should implement the copy assign operator as well then. 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. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:98: buffer.write(reinterpret_cast<const char*>(value.c_str()), sizeof(T::value_type) * length); On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > AFAIK, only std::basic_string has c_str(). So no need for the template then. See above - both std::string and std::wstring are being used here.
http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:32: int32_t type; On 2013/06/03 11:53:00, Wladimir Palant wrote: > On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > > Should be ValueType, not int32_t I guess? > > 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. Fair enough. But I'd want a typedef then :) http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:45: value.assign(data.get(), length); On 2013/06/03 11:53:00, Wladimir Palant wrote: > See above - this function is used both for std::string and std::wstring. I thought basic_string was a base class, but it's a template. So this is fine. http://codereview.adblockplus.org/10824027/diff/1/src/shared/Communication.h#... src/shared/Communication.h:76: OutputBuffer(const OutputBuffer& copy) : buffer(copy.buffer.str()) {} On 2013/06/03 11:53:00, Wladimir Palant wrote: > On 2013/06/03 11:25:41, Felix H. Dahlke wrote: > > We should implement the copy assign operator as well then. > > 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. 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).
I added typedefs and a non-implemented assignment operator.
LGTM and one nit. http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication... src/shared/Communication.h:123: OutputBuffer& Write(const T value, ValueType type) Shouldn't value be passed by reference?
http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication... src/shared/Communication.h:123: OutputBuffer& Write(const T value, ValueType type) On 2013/06/03 12:23:54, Felix H. Dahlke wrote: > Shouldn't value be passed by reference? Given that this function is meant for primitive types only (ints and bools), do you think that this would be wise?
LGTM http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/10824027/diff/8001/src/shared/Communication... src/shared/Communication.h:123: OutputBuffer& Write(const T value, ValueType type) On 2013/06/03 12:25:55, Wladimir Palant wrote: > On 2013/06/03 12:23:54, Felix H. Dahlke wrote: > > Shouldn't value be passed by reference? > > Given that this function is meant for primitive types only (ints and bools), do > you think that this would be wise? Right, didn't think of that. Won't hurt, but it won't hurt right now either. 
|
