Fix warnings in float/double functions #381
Conversation
oschwald
commented
Jun 2, 2025
oschwald
commented
- Do not rely on volatile for double/float decoding
- Improve endian check if macro is not set
I believe volatile was used to ensure that the compile does not optimize
the code in ways that break the bit manipulation. Switching to memcpy
and using the built-in bswap implementations when available seems
like a better way to solve the issue.
This also fixes the following warning:
```
/home/greg/MaxMind/MaxMindDBSwift/Sources/CLibMaxMindDB/libmaxminddb-1.12.2/src/maxminddb.c:1781:12: warning: passing 'volatile uint8_t *' (aka 'volatile unsigned char *') to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
1781 | memcpy(q, p, 4);
| ^
/usr/include/string.h:43:39: note: passing argument to parameter '__dest' here
43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
| ^
/home/greg/MaxMind/MaxMindDBSwift/Sources/CLibMaxMindDB/libmaxminddb-1.12.2/src/maxminddb.c:1799:12: warning: passing 'volatile uint8_t *' (aka 'volatile unsigned char *') to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
1799 | memcpy(q, p, 8);
| ^
/usr/include/string.h:43:39: note: passing argument to parameter '__dest' here
43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
| ^
2 warnings generated.
```
To make nested ifs easier to read
| #elif __has_builtin(__builtin_bswap32) | ||
| return __builtin_bswap32(x); | ||
| #else | ||
| x = ((x << 8) & 0xFF00FF00) | ((x >> 8) & 0x00FF00FF); |
There was a problem hiding this comment.
I'm thinking it wouldn't matter here, but would we want to mask and then shift? I was reading https://justine.lol/endian.html which gave me this idea. It's also what we do in the 64bit case, so maybe it'd be nice to be consistent. I'm ok as is though if you prefer, not sure there's a great reason to change really.
I wonder which compilers will hit these non-builtin cases. Are we hitting it in tests at all? I'd guess not.
On swapping, the answer here seems interesting.
There was a problem hiding this comment.
I don't think we will hit the non-built-in case in CI. I did test it by itself.
For the byteswap.h fallback, it seems likely that any platform that has that would also have the built-ins.
I went back and forth on different implementations. I don't know t hat signedness is a concern here. Variations of the implementations I used are used in many places, e.g., libtrace, fio, boost, ruby, and libwebp. In terms of what is the best implementation, I haven't done any extensive testing. I saw various pages arguing for various implementations, but none of them seemed particularly reputable. Hopefully it won't matter for almost all users.
There was a problem hiding this comment.
I see. So many variations! Sounds good to leave it then.
