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 <[email protected]>
|
Before I go on I should really add a big endian CI here. |
Avoids having to deal with endianness. Signed-off-by: Rosen Penev <[email protected]>
We can just treat the data as big endian with bit shifting and ORing. Signed-off-by: Rosen Penev <[email protected]>
It's the same function. Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
|
removed bad commits. This was tested on the big endian CI. |
No description provided.