Skip reassignment of the initialization value

[irwir]

This was incorrect: Reversed condition changed to less instead of less or equal because the value still should be PNG_UINT_31_MAX.
Reassignment of the same value is a common kind of copy-paste errors.
Hence static analysis emits warning in this case.
A better way to avoid reassignment is in the updated commit.

[jbowler]

I don't see any reason for such a change. The compiler can work it out too and making the change requires a code review which explains why the change from:

if (a > b)
  spurious assignment;
else if (a <= b)
  required assignment;

To:

if (a < b)
   required assignment;

Is correct. I couldn't work it out; why is the case "a == b" irrelevant?

However, back to the words of the pull request rather than the code; why is this better documentation of what the code does than the original form, which you present as equivalent (ignoring the "a == b" issue)? It seems to me that "if/else" is always more clear ab initio than "if".

[irwir]

Thanks for reviewing.
I changed the code and the description in the first message.

[jbowler]

libpng-ng comment: libpng(ng) shouldn't even contain this code for IDAT There is no point detecting an over-long IDAT unless it exceeds the PNG spec length (indicating a corrupted stream). IDAT buffering needs to be controlled internally to optimize the specific zlib implementation (if known) and the architecture performance; small buffers or large buffers depend on the relative speed of the zlib window copy vs the libpng row handling for large images and for small images the code can avoid the window copy entirely if enough is known about the zlib implementation.

[irwir]

As could be read, at least one more version of 1.6 was planned. So this code might stay for some time, while 1.7 branch did not pan out.

PS. About 5 years ago in one discussion it was suggested to rename libspng to libpng-ng. It did not happen.

[jbowler]

@ctruta: won't fix

[ctruta]

@ctruta: won't fix

"If It Ain't Broken Don't Fix It" and yet the code patched by @irwir is neither "broken" nor "not broken".

We work in the C89 mode and the C89 mindset in which variable declarations and variable definitions need not be co-located. (But they should be from C99 onward, and they must be in C++ and Rust and any other RAII language.)

Moreover, I actually like the way in which @irwir is using the ternary operator. It is more obvious (if only slightly more obvious) that the code pattern looks like an initialization of idat_limit with a hand-coded min function, except... it isn't.

It is for this reason I am saying that the code in question is neither "broken" nor "not broken". It looks like an accidental typo. It makes no difference semantically if we replace PNG_UINT_32_MAX with PNG_UINT_31_MAX, or if we don't, but... if we don't, then we should at least add in a comment in which we state that we intentionally don't and here is why. My personal preference would be not to add any comment, but rather, to replace PNG_UINT_32_MAX with PNG_UINT_31_MAX instead.

@irwir if you do modify your code further, as I'm suggesting here, I will appreciate it. But then, if you could please add your name -- your real name -- to the AUTHORS file in your updated commit, that'd be great!

[irwir]

RAII is not a must in C++, but a good practice in many cases.
The orignal code also follows RAII guidelines, and the only purpose of this PR was to eliminate the warning "assigned value was never used" without any deeper code investigation.
If there are other issues with the code, an appropriate fix should be applied.

As for 32 vs 31, if neither png_ptr->width nor png_ptr->channels are zeros (not allowed in PNG specification?), the factor would be at least 2. Therefore, it was most probably intentional and needs no changes.
What further changes could be considered:

• maximum allocation size could be above 32-bit value in 64-bit mode;
• user's limits might be taken into accout - such as PNG_USER_CHUNK_MALLOC_MAX, PNG_USER_HEIGHT_MAX or PNG_USER_WIDTH_MAX.