-
Notifications
You must be signed in to change notification settings - Fork 592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch IceInternal::Buffer underlying type to std::byte #1928
Conversation
scope, | ||
seq->typeMetaData(), | ||
typeCtx | (inWstringModule(seq) ? TypeContext::UseWstring : TypeContext::None)); | ||
string s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeToString
has no context (AFAIT) to detect the type is for a sequence. I thought of adding another type context, but ultimately decided against this since it's only relevant in a few specific places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix the UTF8-related code in a follow-up PR, and remove some related reinterpret_cast.
cpp/include/Ice/OutputStream.h
Outdated
@@ -325,12 +325,12 @@ namespace Ice | |||
assert(v >= 0); | |||
if (v > 254) | |||
{ | |||
*dest++ = std::uint8_t(255); | |||
*dest++ = std::byte(255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use {255}.
@@ -1174,12 +1180,18 @@ Ice::InputStream::read(wstring& v) | |||
if (_instance) | |||
{ | |||
const WstringConverterPtr& wstringConverter = _instance->getWstringConverter(); | |||
wstringConverter->fromUTF8(i, i + sz, v); | |||
wstringConverter->fromUTF8( | |||
reinterpret_cast<const uint8_t*>(i), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix fromUTF8 to take bytes like other APIs.
@@ -39,7 +39,7 @@ namespace | |||
// | |||
// Return unused bytes | |||
// | |||
_stream.resize(static_cast<size_t>(firstUnused - _stream.b.begin())); | |||
_stream.resize(static_cast<size_t>(firstUnused - reinterpret_cast<uint8_t*>(_stream.b.begin()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix the UTF8Buffer API to use bytes.
scope, | ||
seq->typeMetaData(), | ||
typeCtx | (inWstringModule(seq) ? TypeContext::UseWstring : TypeContext::None)); | ||
string s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine.
This PR implements #1825, namely
sequence<byte>
tostd::vector<std::byte>
by default