Re: [PR] THRIFT-5941: Add Ruby ext cppcheck coverage [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G commented on code in PR #3387:
URL: https://github.com/apache/thrift/pull/3387#discussion_r3076015272


##########
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:
   Why do we change that to `uint64_t`? That introduces a lot of implicit things going on. Do we really want that?



##########
lib/rb/ext/compact_protocol.c:
##########


Review Comment:
   cast to int32_t seems stale: 
   - read_varint64() returns uint64_t
   - zig_zag_to_int() expects uint32_t
   



-- 
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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org