kpumuk commented on code in PR #3387:
URL: https://github.com/apache/thrift/pull/3387#discussion_r3076433756
##########
lib/rb/ext/compact_protocol.c:
##########
@@ -419,17 +419,17 @@ static char read_byte_direct(VALUE self) {
return (char)(FIX2INT(byte));
}
-static int64_t zig_zag_to_ll(int64_t n) {
- return (((uint64_t)n) >> 1) ^ -(n & 1);
+static int64_t zig_zag_to_ll(uint64_t n) {
+ return (int64_t)((n >> 1) ^ (0ULL - (n & 1ULL)));
}
-static int32_t zig_zag_to_int(int32_t n) {
- return (((uint32_t)n) >> 1) ^ -(n & 1);
+static int32_t zig_zag_to_int(uint32_t n) {
+ return (int32_t)((n >> 1) ^ (0U - (n & 1U)));
}
-static int64_t read_varint64(VALUE self) {
+static uint64_t read_varint64(VALUE self) {
Review Comment:
With the last amend:
* All conversions are explicit, including (potentially unnecessary `seqid`
overflow, mirroring login in Ruby)
* Separated int32 from int64 path via `read_varint32` / `read_varint64`
* ZigZag decode uses unsigned ints
Still lacking:
* Size enforcement (C++ enforces 5 bytes for int32, 10 bytes for int64,
binary/name length to `INT32_MAX` via signed out `int32_t` and `>= 0`
comparison). Semantically, we should fail on `> INT32_MAX`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
