Skip to content
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.

Sign up for GitHub

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

Support build cython codes with -Werror=strict-aliasing #247

Merged
merged 1 commit into from
Mar 5, 2024
+7 −2
Merged

Support build cython codes with -Werror=strict-aliasing #247

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions thriftpy2/protocol/cybin/cybin.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import sys

from libc.stdlib cimport free, malloc
from libc.stdint cimport int16_t, int32_t, int64_t
from libc.string cimport memcpy
from cpython cimport bool

import six
Expand Down Expand Up @@ -99,7 +100,9 @@ cdef inline int write_i64(CyTransportBase buf, int64_t val) except -1:


cdef inline int write_double(CyTransportBase buf, double val) except -1:
cdef int64_t v = htobe64((<int64_t*>(&val))[0])
cdef int64_t v
memcpy(&v, &val, 8)
v = htobe64(v)
Comment on lines +103 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of memcpy in write_double correctly addresses the strict-aliasing violation by safely copying the double value. It's a good practice to add a brief comment explaining the reason behind using memcpy here, for the benefit of future maintainers.

+    # Using memcpy to comply with strict-aliasing rules
     memcpy(&v, &val, 8)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cdef int64_t v
memcpy(&v, &val, 8)
v = htobe64(v)
cdef int64_t v
# Using memcpy to comply with strict-aliasing rules
memcpy(&v, &val, 8)
v = htobe64(v)

buf.c_write(<char*>(&v), 8)
return 0

Expand Down Expand Up @@ -269,6 +272,7 @@ cdef c_read_val(CyTransportBase buf, TType ttype, spec=None,
cdef int size
cdef int64_t n
cdef TType v_type, k_type, orig_type, orig_key_type
cdef double double_value

if ttype == T_BOOL:
return <bint>read_i08(buf)
Expand All @@ -287,7 +291,8 @@ cdef c_read_val(CyTransportBase buf, TType ttype, spec=None,

elif ttype == T_DOUBLE:
n = read_i64(buf)
return (<double*>(&n))[0]
memcpy(&double_value, &n, 8)
return double_value
Comment on lines +294 to +295
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of memcpy for handling T_DOUBLE in c_read_val is correctly implemented to avoid strict-aliasing violations. Consider adding a comment here as well, explaining the use of memcpy for future code maintainers.

+    # Using memcpy to safely convert int64_t to double, adhering to strict-aliasing rules
     memcpy(&double_value, &n, 8)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
memcpy(&double_value, &n, 8)
return double_value
# Using memcpy to safely convert int64_t to double, adhering to strict-aliasing rules
memcpy(&double_value, &n, 8)
return double_value


elif ttype == T_BINARY:
size = read_i32(buf)
Expand Down
Loading