Handle basic_string in datastream serializing/deserializing #118
Conversation
| template<typename Stream, typename T> | ||
| datastream<Stream>& operator << ( datastream<Stream>& ds, const std::basic_string<T>& s ) { | ||
| ds << unsigned_int( s.size() ); | ||
| for( const auto& i : s ) { |
There was a problem hiding this comment.
Wouldn't datastream.write and datastream.read below be better?
There was a problem hiding this comment.
I checked in terms of performance and (if I'm reading assembler code correctly) there should be no difference for operator<< with above for loop vs ds.write - both end with a single call to memcpy (when compiled with -O2 flag)
From readers perspective ds.write/read is a bit more concise so I have changed the code.
basic_string operator<< with for loop:
basic_string operator<< with ds.write:
There was a problem hiding this comment.
Maybe change it back then. Otherwise the write() needs sizeof I believe.
There was a problem hiding this comment.
operator>> and ds.read needs sizeof(T), but sizeof is a compile time thing (there is no runtime cost) so I wouldn't worry about it.
I think it is only matter of what is easier to read by developer.
| // std::basic_string | ||
| ds.seekp(0); | ||
| fill(begin(datastream_buffer), end(datastream_buffer), 0); | ||
| static const std::basic_string<uint8_t> inputBasicString {0, 1, 2, 3, 4, 5}; |
There was a problem hiding this comment.
please add test with 2+ bytes type. something like wchar_t
There was a problem hiding this comment.
That was really, really good idea.
It occurred that the second solution with datastream.read/write was wrong. Probably because casting to char* was narrowing 2 bytes to 1 byte?
I reverted solution to the original one and it looks good.
Thanks!
There was a problem hiding this comment.
I think your issue was that you had * sizeof(T) in read but not in write. I can't think of other reason but if wouldn't work, try to use static_cast instead of c-cast. We have std::string that is using read/write. I propose to have consistent code. I see that -o2 supposed to make no difference here but anyway that is performance critical code so I think we should use read/write to make even non-optimized code faster.
There was a problem hiding this comment.
My code is copied from vector serializing so I believe it is consistent (because basically basic_string is a container).
What is the purpose of making non-optimized code faster? The difference would be only if someone deliberately builds code with debug flags. If it really is important then we need to create issues for improving both vectors and arrays serialization.
There was a problem hiding this comment.
I think your issue was that you had
* sizeof(T)inreadbut not inwrite.
Thanks, that was the issue.
I additionally changed char* to void* in read method, so I could avoid unnecessary reinterpret_cast<char*> from wchar_t* in my code (which shouldn't be needed anyway, because memcpy needs void* type).
There was a problem hiding this comment.
Short answer - I don't know for sure. My comment was based on the fact that cdt is not most recently installed compiler. this is our own patched compiler and its main purpose is to compile to web assembly. So I think it is easier to make it just optimal than check this behavior in wasm.
Regarding having optimal debug build - that may be helpful to avoid timeouts while we debug. You may recall errors in DUNE due to performance. Imagine you have slow machine and debug builds, it will be even slower so you need additional configuration to allow big timeouts
…ly wide characters. Add unit tests for 2 byte wchar_t.
mikelik
force-pushed
the
mikelik/basic_string
branch
from
060000a to
da5547d
Compare
Change Description
Resolves #103
Fixes crash in cdt-cpp when using a std::basic_string<> in action wrapper
API Changes
Documentation Additions