stb_image: unused functions warning when using `STBI_ONLY_PNG`

[rajveermalviya]

Describe the bug

when using STBI_ONLY_PNG and cc -Wall, compiler generates following warnings:

stb_image.h:1068:12: warning: unused function 'stbi__addints_valid' [-Wunused-function]
static int stbi__addints_valid(int a, int b)
           ^
stb_image.h:1076:12: warning: unused function 'stbi__mul2shorts_valid' [-Wunused-function]
static int stbi__mul2shorts_valid(short a, short b)
           ^
2 warnings generated.

https://godbolt.org/z/o535xjnWs

To Reproduce

#define STB_IMAGE_IMPLEMENTATION
#define STBI_ONLY_PNG
#include "stb_image.h"

Compiler Info

$ clang --version
clang version 15.0.7 (Fedora 15.0.7-1.fc37)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

[rygorous]

These are not going to get fixed.

The STBI_NO_* and STBI_ONLY_* defines are a historical mistake that we still keep in the library purely to not break existing users, but there are 9 binary flags here that work out to 512 possible combinations (511 if you exclude the nonsensical "support no formats" configuration), and that is simply too much of a maintenance burden to keep compiling warning-free across multiple versions of the 3 major compilers we support, especially considering how low-value the feature is.

Individually each such flag combination works out to a few #if directives, but cumulatively it leads to all shared code looking like https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_image.h#L1020 which is just a brittle nightmare. Unused static functions might elicit warnings but they are completely harmless. Copy-pasting these #ifs around everywhere hurts readability and maintenance, and getting them wrong in the other direction and excluding code that shouldn't be (and what should and shouldn't be excluded can easily change for only some combinations of flags e.g. after a security patch) will cause the library to not compile, which is a much more serious issue.

For the time being, we'll keep the ability to disable individual formats (to not break existing code), and we also do our best to keep the code warning-free in the "stock" configuration at reasonable warning levels for the big three compilers (Clang, GCC, MSVC up to -Wall / -W4 but not including things like -Wextra), but warning-free-ness across the many possible combinations of enabled formats is a Sisyphean exercise that actively makes the code more brittle and harder to maintain from our end. We have a very limited amount of spare time for this project and this is not a good use of it.

[N-R-K]

but cumulatively it leads to all shared code looking like

https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_image.h#L1020 which is just a brittle nightmare.

That is correct. However, there's a simpler alternative:

#if __GNUC__
  #pragma GCC diagnostic push /* also works on clang */
  #pragma GCC diagnostic ignored "-Wunused-function"
#endif

// implementation here

#if __GNUC__
  #pragma GCC diagnostic pop
#endif

This should take care of gcc and clang, without turning the warning off for user-code.

There might be some gotcha to this that I'm missing, so I'm not necessarily saying this is the right way to go. Rather, I'm just pointing it out as an alternative in case you haven't considered it.

[rygorous]

We're trying not to get into warning disable pragmas in STB code either because these, too, have been the source of problems in the past, specifically with older compiler versions not having the warnings we want to disable in the first place and complaining about unknown options, or in some cases warnings being moved between categories across versions, or push/pop being supported in newer versions but not older ones (or inconsistently in other ways). Plus we do want the unused function warnings in some cases (especially in the stock configuration with everything enabled), just not when functionality is selectively disabled. (This too could be done with even more compile-time #ifs but as explained above, I'd rather not support it at all.)

These days we generally avoid everything along those lines.

Part of the point of the single-header-file library model is that you are fully in charge of the build. If the warnings bug you, you can just disable them in your build setup, either globally or in the file that defines STB_IMAGE_IMPLEMENTATION using the appropriate pragmas for whatever toolchain you are actually using. This is much easier to deal with (and less error-prone) at the "consumer" end than it is for us, because you know what compiler and toolchain you're using and we don't.

The warning thing is just on particular instance of the whole "non-standard config options" x "rare build environment" matrix that is, overall, a frequent and steady source of bug reports caused in large part by us too readily accepting patches that reduce in a combinatorial explosion of build configurations that we have no sane way to support with the amount of time we have.

We're in this hole now whether we like it or not, but we have to start somewhere, and I've decided this means that:

• I will not accept future patches that multiply the number of configurations in this way
• I will fix regressions (actual bugs or compilation errors) even for obscure targets (or ancient toolchains) when they used to work
• I will no longer take any non-trivial patches to add support for new rare platforms. (This kind of thing: https://github.com/nothings/stb/commit/038b6ab9ec485423c0231913b5b3bf1184e48f2a I'll still take but if it had been more than 2 lines I probably wouldn't have.)
• I will no longer take patches that try to make baseline C standard or math library functionality optional, the one exception being some facility to replace malloc/free/realloc with a compatible replacement which is a common ask and very localized. But things like no stdio, avoiding memset/memcpy, or avoiding standard math functions (the #define to avoid fabs usage in stb_dxt takes the cake on that one) cause problems at a rate entirely disproportionate to how often they are actually used. If you want that kind of thing, feel free to adapt the STB libs for your needs but do so in a private fork.
• I will take patches that fix warnings on typical compilers on typical warning levels, but many -Wextra or static analyzer warnings firmly fall into "matters of taste" territory, and we've been in the situation more than once where a suggested fix for one compiler than caused a new warning on a different compiler; likewise, at a certain point such warnings are mostly false positives, and they're also often subject to frequent change with compiler or analyzer versions. When a warning or analyzer flags something that looks like an actual issue we're happy to fix it, and we also try to keep everything compiling cleanly at default settings, but fixing warnings just to fix warnings is a non-goal.

As I'm writing this, I realized we should probably post that somewhere more visible. :)

[nothings]

To clarify one thing, my policy on warnings has always (or at least for a long time) been to fix normal warnings but not warnings triggered by -Wextra, and things only enabled by -Wall have been borderline.

But I wouldn't generally say "If the warnings bug you, you can just disable them in your build setup", because we would like the stb libraries to be easy to use, and forcing you to tweak warnings, especially if it affects the rest of your code, isn't cool. BUT we do have to draw the line somewhere, so generally anything -Wextra or otherwise requiring manual enabling is a warning we won't fix, unless it's exceptionally easy in terms of future maintenance (e.g. doesn't add any extra lines of code).