Code clean-up and simplification

[nayuki]

I apologize for the complexity of processing this set of commits.

Back on 2021-09-12, after emailing Greg Roelofs, I imported the publicly available source code of pngcheck at version 3.0.3 (released on 2021-04-25). Evidently, other people have imported that code and ended up with the exact same working tree but with different commit metadata: a9cf3731 by Nayuki, cab5ffb1 by svgeesus, 67d38027 by Chris Lilley. You can confirm the matches by performing git diff <CommitHash0> <CommitHash1> and seeing zero output.

From that initial state, I made 21 changes, and I'll let the commit messages speak for themselves (oldest to newest):

0. Added authorship information.
1. Fixed indentation to eliminate tabs and consistently use 2 spaces.
2. Changed function signatures to take const char pointers for safety.
3. Changed function signatures to take const pointers when possible for safety.
4. Gave name to an anonymous enum, changed variable types to match.
5. Converted struct printbuf_state to use bool members instead of int.
6. Converted command-line flag global variables to be bool instead of int.
7. Converted pngcheck()'s have_ variables to be bool instead of int.
8. Converted pngcheck()'s last_is_IDAT and last_is_JDAT variables to be bool instead of int.
9. Converted pngcheck()'s miscellaneous variables to be bool instead of int.
10. Converted check_ascii_float()'s relevant variables to be bool instead of int.
11. Converted miscellaneous variables to be bool instead of int.
12. Converted two functions' parameters to be bool instead of int.
13. Converted chunk type bit-testing macros to functions for safety.
14. Reordered declarations pertaining to global error status.
15. Converted global-error macros to functions for safety.
16. Converted isASCIIalpha() from macro to a renamed function, tweaked parameter type and declaration ordering.
17. Replaced latin1_keyword_forbidden table with a renamed function.
18. Replaced latin1_text_discouraged table with a renamed function.
19. Added no-op casts to clarify signed-unsigned comparisons and suppress compiler warnings.
20. Simplified gcf() while producing identical results.

The result of my original batch of work is at https://github.com/nayuki/pngcheck/tree/ea8192e360e6b13b6699908b2f5198395a24ca21 . The changes amount to improvements for human readability and semantic clarity, but don't introduce any new functionality. I confirm that the code compiles with no errors in GCC and LLVM.

I just discovered this project repository ( https://github.com/pnggroup/pngcheck ) today and it looks official enough that it could host the work that I did. To that end, I rebased my commits so that the starting point is not a9cf373 but instead cab5ffb (which as I said before, has the exact same working tree). The new tip of my development branch is e2cfeeb0 (patch-0), and is what I am proposing in this pull request.

I understand that this has merge conflicts with the current main commit and probably doesn't have a simple resolution. I understand that while each of my commits is simple and self-contained, the cumulative result of all of them is not easy for you to handle.

Let me know if this work is acceptable, and whether I can do anything to help get it integrated. Thank you for your consideration.

[svgeesus]

@jbowler your comments on these proposed changes would be most welcome, especially as there are conflicts.

[svgeesus]

@nayuki thanks for unearthing these. They look like useful changes to move this elderly codebase to a more maintainable format.

It would be simpler to handle them as individual PRs, especially as some have already been done.

[jbowler]

@svgeesus most likely the char*->const char* change will conflict with mine but as I said it's a copy edit. If that's the only conflict do a git revert on it (it's currently at HEAD), git merge then if you want my own commit should rebase and result in an empty commit.

@nayuki: you should verify using the changes with the CFLAGS (in particular CERRORS) that I added to Makefile.unx. I believe those should work for both the GCC and LLVM.

In general: a quick review for the changes as described (i.e. without looking at the source code) suggests those changes are all good. They cover many of the things I was thinking of doing but haven't done. There are a couple of caveats below but the only major issue I see is the introduction of (cast) to solve what I think you (@svgeeus) have already solved with your own changes plus the missing one I mentioned before.

There is a policy issue here: I never use (cast) to hide a compiler warning unless there is no other way and I thought you (@svgesus) had fixed those without cast by correcting the use of ulg. This is a much, much, much better solution so the last but one commit should go. It's policy because when I do have to use a (cast) I add the annotation /*SAFE*/ but Cosmin seems to have a problem with that. I think of casts as the three monkeys' approach to error messages.

There is a second policy issue surrounding indentation. This is purely stylistic and since I've contributed to a few FOSS projects and worked (many years ago) in different corporate environments I just try to copy the style. I hope most software developers do that, however the style has to be documented, clearly and, preferably, supported by editor settings. Those should apply cross-organisation. Cosmin added .editorconfig which will work in this case but probably requires a major re-indent. That should be done in one step when there is no active development.

What I suggest @svgeesus is that you give @nayuki the privileges to at least create a branch in pnggroup/pngcheck and make this the development branch. The constraint here is that because pngcheck.c is a single file it's not viable to have parallel development; individual chunk support can be added but changes to the core functionality and the control loop like #23 need to be done by just one developer (who needs to be in charge). I'm certainly not volunteering :-)

I'll separate detailed comments out.

[nayuki]

@jbowler Thanks for your thoughts. A partial reply:

conflict ... git merge ... rebase

In the worst case, I could restart with the HEAD/tip of development that you would specify, and painstakingly re-apply each of my changesets individually and deal with any problems that materialize (merge conflicts, semantic errors, additional changes on code that was not present when I made the original changes).

the only major issue I see is the introduction of (cast)

I agree that this technique is fishy. If you look at my changeset, you can see that I'm casting the value ityp back to uch, which is what it came from a line ago. It is the most minimal change that is still clear and correct. Note that the variable int ityp is declared at the top of pngcheck() which is 200 lines up. Another way to fix the problem is to assign buffer[9] to a new variable of a more suitable type, but that would mean changing more of the existing code.

I don't think there is a clean way to deal with this situation - whether modifying existing code, adding new code, or ignoring the signed-unsigned compiler warnings. It's a matter of trade-offs and which type of ugliness you prefer.

There is a second policy issue surrounding indentation. This is purely stylistic and since I've contributed to a few FOSS projects and worked (many years ago) in different corporate environments I just try to copy the style.

I understand. I am extremely strict on indentation in my own projects, but I am way more lenient on other people's projects. If you look at the changeset, I changed just 14 lines in the 5200-line file. The code had inconsistent indentation to begin with. I was fixing the tiny amount of inconsistency in favor of the overwhelming majority style (2 spaces per indent) already present in the file.

What I suggest @svgeesus is that you give @nayuki the privileges to at least create a branch in pnggroup/pngcheck and make this the development branch.

As I don't expect to do ongoing development on pngcheck, I'm okay if you don't give me commit privileges in your repository. Thank goodness this is not the CVS or SVN days. I'm totally okay with you plucking changes from my copy of the repo.

[jbowler]

Detailed comments.

I'm assuming, @nayuki, that your comments accurately summarise all the actual changes. I'm not sure I see any need to do line-by-line review and not doing it avoids getting lost in purely stylistic issues; what I call code correctness. In addition the code as it stands is at times very difficult to understand (see my comments in #23) and that makes it difficult to review line-by-line.

Build failures:

missing-prototypes: I get errors for "-Werror=missing-prototypes" (you must either declare a prototype with extern or use static; I suggest the latter).

type-limits: -Wtype-limits: comparing (unsigned) >= 0 line 311.

discarded-qualifiers: assigning string literals to (char *): the auto should be (const char *). Missing in your own const changes (it's there in mine; I just changed every string "char *" to "const char *"!)

Individual changes:

• Added authorship information.

• Fixed indentation to eliminate tabs and consistently use 2 spaces.

  • Policy issue: libpng (therefore at present pnggroup) uses 3 spaces. It should be consistent otherwise it will get messed up.

• Changed function signatures to take const char pointers for safety.

• Changed function signatures to take const pointers when possible for safety.

• Gave name to an anonymous enum, changed variable types to match.

  • Yes; the more const the better but the auto variables need to be changed to (as in the build failures above.)

• Converted struct printbuf_state to use bool members instead of int.

• Converted command-line flag global variables to be bool instead of int.

• Converted pngcheck()'s have_ variables to be bool instead of int.

• Converted pngcheck()'s last_is_IDAT and last_is_JDAT variables to be bool instead of int.

• Converted pngcheck()'s miscellaneous variables to be bool instead of int.

• Converted check_ascii_float()'s relevant variables to be bool instead of int.

• Converted miscellaneous variables to be bool instead of int.

• Converted two functions' parameters to be bool instead of int.

  • You added <stdbool.h> I know Cosmin wants to do that but it requires C99 and is deprecated in C23. _Bool is the actual type until C23 which adds bool as a real type (rather than a macro). @svgeesus; this is part of the big policy beef I have with pnggroup; there is no documentation of the C standard that must be supported other than C90/C89. I've been trying to get this fixed for, I don't know, a couple of years but no one wants to make a statement. As it stands @nayuki's patches contain a significant number of things that don't compile with C90 (including a C99 style comment) so I feel this needs to be fixed at org level.

• Converted chunk type bit-testing macros to functions for safety.

• Reordered declarations pertaining to global error status.

• Converted global-error macros to functions for safety.

• Converted isASCIIalpha() from macro to a renamed function, tweaked parameter type and declaration ordering.

• Replaced latin1_keyword_forbidden table with a renamed function.

• Replaced latin1_text_discouraged table with a renamed function.

  • Sounds good but the latin1 functions are generating the type limit warning I mentioned above. Possibly this is something Chris fixed in his changes. I recommend that functions like this are marked static. This is partly stylistic but in reality static obviates inline.
  • I'd also like to suggest that the global variables all be marked static. A good compiler can simply remove them from the build if they are not required.

• Added no-op casts to clarify signed-unsigned comparisons and suppress compiler warnings.

  • These are not necessary and Chris's changes eliminate them. You also missed one; the chunklen > incnt test on line 333 of pngsplit.c

• Simplified gcf() while producing identical results.

Conclusions:

Mostly harmless so far as I can see. It is worth noting that changes which alter the UI need a bump to a major version. That could be "3.1" or it could be "4.0". So far as I can see these changes don't require that but the exit code change I suggested in #23 would.

[nayuki]

@jbowler Thanks for the more detailed review and context (e.g. the bool policy). Overall, it sounds like cherry-picking my changes would be appropriate, as there are changesets that should be dropped (such as my incomplete refactoring of const) or heavily modified for various reasons.

missing-prototypes: you must either declare a prototype with extern or use static

I wasn't aware of this issue back then, so it wasn't part of my original changesets. I'm happy to deal with this issue in the rewrite.

Policy issue: libpng (therefore at present pnggroup) uses 3 spaces.

Well, I don't see that policy in force even in the current version of the code. Also, I have to voice my complaint that 3 spaces is a highly unusual policy (usually 2, 4, or tab) that I've never seen any project use, and is also aesthetically unpleasing IMO. I'm not going to stop you from doing it, though.

the auto variables need to be changed to (as in the build failures above.)

I don't see any use of auto. Did I misunderstand something?

@nayuki's patches contain a significant number of things that don't compile with C90 (including a C99 style comment)

I should note that the code already had one C99 comment before I started, so I didn't make things worse: //have_DHDR = 1;. It's even in your code!

pngcheck/pngcheck.c

Line 3879 in 194a7fd

//have_DHDR = 1;

You also missed one; the chunklen > incnt test on line 333 of pngsplit.c

I wasn't interested in pngsplit.c because I haven't used it, so I'm not even aware of what the program does. So I didn't make any changes there; only made changes in pngcheck.c.

[jbowler]

Some clarifications:

1. auto - I mean a function-local definition/declaration. Traditionally auto isn't used much because it is only required if the type of the variable is not given and then it resulted (traditionally) in (int). However C23 has type deduction (not sure what the official term is) so this compiles with -std=c23 -pedantic:

int f(const char *s, int lim) {
    auto i=0;

    for (auto siter=s; *siter != 0; ++s)
        ++i;

    return i;
}

Expect to see much more use of explicit auto :-)

2. The (cast) thing is something of a religion for me having seen too much code broken by bad casting. As I said it's style/policy.

3. ityp is the PNG "colortype" and in branch main it's declared as (unsigned) The buffer casting (to (uch)) is annoying, maybe ityp should be (char). Details: my approach has typically been to wrapper fread and fwrite with (void*) functions and then pass in (png_byte) buffers.

4. If you do a git rebase it will take you through every conflicting patch one-by-one. A merge is required, whether by rebase or otherwise, because your changes don't have Chris's added chunk support.

I guess from my point of view the bottom line @svgeesus is that this is desirable code cleanup some of which has already been done in the main branch and some of which raises policy issues (bool in particular). On the other hand the main branch includes development work (your own) to add the new chunk support and that is much more important.