Fix a bunch of warnings#4925
Conversation
This change fixes a bunch of warnings that were discovered by compiling with `clang -target=i386-pc-linux-gnu`. It turned out that the intrinsics were not necessarily being used in all platforms! Especially in GCC, since it does not support __has_builtin. Some more warnings were gleaned from the Windows build, but I stopped when I saw that some third-party dependencies (e.g. zlib) have warnings of their own, so we might never be able to enable -Werror there.
| (defined(__GNUC__) && (__GNUC__ >= 5))) | ||
| # define git__add_sizet_overflow(out, one, two) \ | ||
| __builtin_uadd_overflow(one, two, out) | ||
| __builtin_add_overflow(one, two, out) |
There was a problem hiding this comment.
This intrinsic handles whatever sizes you throw at it: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
But with some odd flag combinations in clang, it requires linking against compiler-rt, which was suboptimal. Keeping the chunk in L58-L63 avoided that case and compiled correctly with the intrinsics in all cases I could think of.
It seems like MingW64's size_t is defined differently than in Linux.
|
Thanks for doing this - there's a lot of unrelated changes in this first commit, and they're not well described, so I'll need a little bit of time to tease things out (ugh all the different That's not meant as a complaint as much as a warning that I'll need some more time to look at this before I merge this, even though it's a rather small PR. |
Less controversial changes together is better.
|
Moved the intrinsics part of the change to #4929 to make it easier to review. |
| # define PRIxZ "Ix" | ||
| # define PRIXZ "IX" | ||
| # define PRIdZ "Id" | ||
| # endif |
There was a problem hiding this comment.
This shouldn't be necessary, Id should be 32-bit on 32-bit platforms and 64-bit on 64-bit platforms. It should be the moral equivalent of zd. From MSDN:
The ptrdiff_t and size_t types are __int32 or unsigned __int32 on 32-bit platforms, and __int64 or unsigned __int64 on 64-bit platforms. The I (uppercase i), j, t, and z size prefixes take the correct argument width for the platform.
|
Appreciate you bumping the intrinsics out, that's definitely helpful for reviewing. I think that the printf formatter values need not change to be |
|
Yes, on Windows (amd64; MinGW): this goes away with the formatting change. let me add a comment. |
|
Cool, thanks for doing this. Looks good to me. |
2 participants
This change fixes a bunch of warnings that were discovered by compiling
with
clang -target=i386-pc-linux-gnu.