various byteSwap and bit_cast cleanups #3227
Conversation
src/bmffimage.cpp
Outdated
| std::array<char, sizeof(uint32_t)> p; | ||
| std::memcpy(p.data(), &n, sizeof(uint32_t)); | ||
| std::string result(p.begin(), p.end()); |
There was a problem hiding this comment.
I think this function might be a cleaner as a loop that mask the bottom byte and then does n >>= 8 on each iteration. That way, there's no need to think about endianness.
There was a problem hiding this comment.
I just tried this.
godbolt output ballooned from 39 lines to 287. The code looks incredibly inefficient with operator new and memcpy being called.
There was a problem hiding this comment.
Second attempt went from 39 to 49. Maybe that's good enough.
edit: now 31.
src/image.cpp
Outdated
| uint16_t v; | ||
| std::memcpy(&v, buf.c_data(offset), sizeof(uint16_t)); |
There was a problem hiding this comment.
The read_uint8 method checks for out-of-bound accesses, so it isn't a good idea to replace it with a memcpy.
It would be better to use methods like read_uint16 and read_uint32 which have a ByteOrder parameter. But this function has a bSwap parameter which doesn't mean the same thing, so a bit of refactoring would be needed.
Turns out that even though MSVC is supposed to support std::byteswap, I can't seem to get it to compile with Godbolt. Whats more, the fallback path MSVC cannot optimize. Signed-off-by: Rosen Penev <rosenp@gmail.com>
|
Before I go on I should really add a big endian CI here. |
Avoids having to deal with endianness. Signed-off-by: Rosen Penev <rosenp@gmail.com>
We can just treat the data as big endian with bit shifting and ORing. Signed-off-by: Rosen Penev <rosenp@gmail.com>
It's the same function. Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
|
removed bad commits. This was tested on the big endian CI. |
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes byte-swapping and binary-to-float conversions and consolidates endian-dependent routines.
- Use
std::bit_castingetDoublewhen available - Centralize byte-swapping via
Image::byteSwapand MSVC intrinsics - Refactor platform-dependent
toAsciiand GUID parsing into explicit byte loops
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.cpp | Added std::bit_cast path under __cpp_lib_bit_cast guard |
| src/pgfimage.cpp | Removed redundant byteSwap_ overload; call Image::byteSwap |
| src/jp2image.cpp | Replaced platform-based reverse with explicit byte-shift loop |
| src/image.cpp | Added MSVC-specific _byteswap_ intrinsics for all word sizes |
| src/bmffimage.cpp | Refactored toAscii to loop with explicit mapping (_, .) |
| src/asfvideo.cpp | Replaced memcpy + conditional swaps in GUIDTag with shifts |
Comments suppressed due to low confidence (4)
src/types.cpp:316
- Add unit tests covering the new
std::bit_castpath ingetDoubleto verify correct behavior for both little- and big-endian inputs on compilers supporting C++20.
#ifdef __cpp_lib_bit_cast
src/asfvideo.cpp:49
- [nitpick] Add a comment explaining that these shifts construct the field in little-endian order to clarify the byte index mapping.
data1_ = (static_cast<uint32_t>(bytes[3]) << 24) | (static_cast<uint32_t>(bytes[2]) << 16) | (static_cast<uint32_t>(bytes[1]) << 8) | (static_cast<uint32_t>(bytes[0]));
src/jp2image.cpp:116
- [nitpick] Consider extracting this byte-to-char mapping logic into a shared helper function, since a nearly identical loop exists in
bmffimage.cpp.
for (size_t i = 0; i < result.size(); ++i) {
src/bmffimage.cpp:89
- [nitpick] The
toAsciiimplementation here mirrorsjp2image.cpp; extracting it into a common utility would reduce duplication and ease future updates.
std::string result(sizeof(uint32_t), '\0');
No description provided.