Closed jbowler closed 7 months ago
Correction: the warning is that on a specific architecture the comparison is always false (the LHS is within range of the data type). This is not true on 16-bit architectures where size_t is 32 bit; the promotion to 32-bit signed works just fine. The compiler can still optimize out the entirely spurious png_warning etc on architectures where it can't happen.
But does it?
@ctruta; can you check the new version carefully for me. I believe the old version simply failed on a 16-bit system. I don't know if 16-bit is still supported but the code as written now should work correctly on any architecture.
I think it should be 'png_alloc_size_t'; i.e. ~(png_alloc_size_t)7 See the comments around line 542 of pngconf.h
@ctruta; can you check the new version carefully for me. I believe the old version simply failed on a 16-bit system. I don't know if 16-bit is still supported but the code as written now should work correctly on any architecture.
The only 16-bit compiler that I have at hand is OpenWatcom, which I haven't set up for verification.
I don't mind continuing the support for 16-bit compilers on a best-effort basis, but the accommodation of NEAR pointers and FAR pointers gotta go. And the associated png_*p
typedef are due for deprecation, right there with their old friends png_const
and png_const_charp
and the rest of the gang. Not deleting them, to be clear, because doing that would break almost the entire world of PNG-supporting implementations. But there should be only one pointer type -- i.e. the standard C pointer type -- going forward.
I'll give a spin to OpenWatcom when I have a chance.
The only support for 16-bit (int) that I could find in the header files seemed to be for TurboC. For sure the far/near support got dropped at some point, I didn't see it when I scanned pngpriv.h but certainly it was gone in 1.7
Nevertheless there is no particular reason why a system with 16-bit (int) should not have 32-bit (void*). Just as on 64-bit architectures. Indeed on the ones I'm familiar with even (long) is 32-bits!
The specific bug was the assumption that 7U had just as many bits as a (png_uint_32), I only used (png_alloc_size_t) to avoid the compiler warning.
I certainly believe that 16-bit (int) while unlikely is something that should be supported, but then I believe libpng should move to C99 to get access to stdtypes.h; this leads to far more clear code though I admit that ~7U would still be a problem. I don't know if C99 gives a concise way of writing constants that have an explicit bit size.
The only support for 16-bit (int) that I could find in the header files seemed to be for TurboC. For sure the far/near support got dropped at some point, I didn't see it when I scanned pngpriv.h but certainly it was gone in 1.7
👍
Nevertheless there is no particular reason why a system with 16-bit (int) should not have 32-bit (void*). Just as on 64-bit architectures. Indeed on the ones I'm familiar with even (long) is 32-bits!
The 32-bit pointer size on 16-bit machines was never a problem.
The problem was about the mixture of 16-bit and 32-bit pointers, which, at the time of the nascent PNG and libpng, used to be business-as-usual. But that thing fell out of favour in the early 2000's, until it officially became a big no-no-no in C++11 when they worked out the move semantics that made the "good" smart pointers possible.
So I'm glad to see that "near/far" pointers are gone from libpng. The next to deprecate are png_structp
and png_infop
and png_const_charp
and all of the remaining superfluous abstractions that start with png_*
and end with *p
.
The specific bug was the assumption that 7U had just as many bits as a (png_uint_32), I only used (png_alloc_size_t) to avoid the compiler warning.
It is a correct workaround. (Thx, BTW.)
I certainly believe that 16-bit (int) while unlikely is something that should be supported, but then I believe libpng should move to C99 to get access to stdtypes.h; this leads to far more clear code though I admit that ~7U would still be a problem. I don't know if C99 gives a concise way of writing constants that have an explicit bit size.
The canonical C99 way to write that expression is ~UINT32_C(7)
, but ~(png_alloc_size_t)7
is a fine solution as well.
The previous fix was GCC specific, this was reasonable when different compilers produced different warning messages but these days it's just source line bloat.