fix(python): divide __slice_len by sizeof(T) to get element count in payload() - #1543
Conversation
| return Slice(self.payload_ptr, self.__slice_len, contained_type) | ||
| # __slice_len is the raw payload size in bytes; divide by the element | ||
| # size to obtain the element count expected by Slice. (fix for #1533) | ||
| number_of_elements = self.__slice_len // ctypes.sizeof(contained_type) |
There was a problem hiding this comment.
This fixes the symptoms but not the actual bug. The problem is that - only in python - the number_of_elements contain wrongly the number of bytes and not how many contained_types are contained in that slice.
There was a problem hiding this comment.
Good catch — renamed the Rust FFI getter from __slice_len to __payload_size_in_bytes across all 9 binding files (Sample, SampleMut, SampleMutUninit, Request*, Response*, ActiveRequest). The unit is now explicit at the source; the element-count division stays in the Python layer where contained_type is known via the type annotations.
While renaming I noticed request_response_extensions.py had the identical byte-vs-element bug at request_payload() and response_payload() — not covered by the original fix. Fixed those too in the same commit (1e909b4).
There was a problem hiding this comment.
But now the whole API is inconsistent to the C++, C and Rust API where this is always the len.
This creates a lot of problems when using this API across language boundaries. In one language the slice size is measured in bytes in the other in elements. Furthermore, such an API doesn't make sense for a high level abstraction.
A list or array len is in python also measured in number of elements and not number of bytes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1543 +/- ##
==========================================
- Coverage 76.60% 76.59% -0.01%
==========================================
Files 452 452
Lines 46188 46214 +26
Branches 1489 1492 +3
==========================================
+ Hits 35382 35398 +16
- Misses 9544 9553 +9
- Partials 1262 1263 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…d_size_in_bytes Per @elfenpiff review on PR eclipse-iceoryx#1543: the previous fix divided bytes by element size at the consumer, leaving the FFI getter misleadingly named `__slice_len` while returning raw bytes. Rename the FFI getter across all 9 binding files (Sample, SampleMut, SampleMutUninit, Request*, Response*, ActiveRequest) to `__payload_size_in_bytes` so the unit is explicit at the source. The element-count division stays in Python where the contained_type is known via type annotations. While here, apply the same element-count fix to request_response extensions (request_payload, response_payload) — they had the identical bug but were not covered by the original eclipse-iceoryx#1533 fix.
|
Option 2 implemented in 91e3c21. The payload element size is read once from each port factory's static config and threaded through:
|
…lice_len The Python bindings' __slice_len returned payload().len(), which for a custom-payload slice is the byte length, so Slice.len() reported N * sizeof(T) instead of N for element types wider than one byte. Read number_of_elements from the sample header instead, which is where the C API takes the same quantity (iceoryx2-ffi/c/src/api/sample.rs), so the Python API is consistent with the C, C++ and Rust APIs: len is the number of elements. The Python layer already passes __slice_len straight to Slice(number_of_elements), so no change is needed there. Regression test: a Slice[c_uint32] of 3 elements must report len() == 3, not 12.
91e3c21 to
54cee65
Compare
|
You refused the byte-based API on 15 April; 96 minutes later we pushed the 23-file element-size threading, which was not what you asked for either, then went silent. This replaces it with what you asked for: |
2 participants
Notes for Reviewer
The Rust FFI layer's
__slice_lenproperty returns the raw byte length of thepayload buffer. The Python
payload()function was passing this byte count directlyto
Slice()as the element count.For single-byte element types (
c_uint8) this happened to be correct. For anyelement type wider than one byte (
c_uint32,c_int64, a struct),Slice.len()would report
N * sizeof(T)instead ofN.Fix: one line in
payload()— divide__slice_lenbyctypes.sizeof(contained_type)before passing to
Slice().New regression test:
test_slice_payload_element_count_is_correct_for_multi_byte_typespublishes a
Slice[c_uint32]of 3 elements and assertsSlice.len() == 3(not 12).Pre-Review Checklist for the PR Author
Convert to draft)iox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)PR Reviewer Reminders
References
Closes #1533
AI-assisted — authored with Claude, reviewed by Komada.