gcc/clang missing-field-initializers warnings for stb_image_write

[flriancu]

Hi @nothings ,

GCC and clang report several missing-field-initializers warnings for stb_image_write. Would you be interested in a PR for this? It would basically replace

stbi__write_context s = { 0 };

with

stbi__write_context s;
memset(&s, 0, sizeof(stbi__write_context));

Thanks!

[rygorous]

I don't get these warnings on Windows with Clang 12, not even if I specifically ask for -Wmissing-field-initializers. Can you be more precise about which compiler version and target you're getting these warnings on?

[flriancu]

@rygorous That was clang 9 or 10. If you can't reproduce with clang 12 nor with mingw gcc latest, I think we can close this issue. If time allows (unlikely), I'll test some more in the following days. If I can't get around to it, feel free to update this ticket as you see fit.

[DctrNoob]

I also get this warning when compiling with clang 12.0.5 and target x86_64-apple-darwin20.5.0.

[solidpixel]

I get warnings when compiling astcenc using clang++-9 and clang++-12 on Ubuntu 20.04 (WSL).

[nothings]

I can make this happen on godbolt for clang 12.0.1 when compiling as C++ (but not as C) if I use -Wmissing-field-initializers, but not with -Wall. https://godbolt.org/z/5j8s87Ta7

@DctrNoob and @solidpixel, please clarify C vs C++ and commandline -W arguments.

[nothings]

My guess is that the logic here is the {0} initializer is a C idiom so clang won't warn in C, but C++ has so many other ways to initialize structs that it's no longer considered an acceptable idiom there. If that's the sort of logic behind this (meaning they're not going to fix it), I guess it might be worth changing. However, if it doesn't happen even with -Wall I don't know if it's really worth it.

[solidpixel]

Can confirm it's C++ (clang++) for my builds.

Team policy is to go strict on warnings, so we're compiling with -Wall -Wextra. Testing locally on clang++-9 it's the -Wextra which is tripping this up, -Wall alone doesn't trigger it.

Given the original code is perfectly fine in C and we're slightly off-piste compiling it as C++, I've just worked around this locally using C++ zero-initializers.

[DctrNoob]

@nothings I can confirm @solidpixel 's finding. I only get the warning when using -Wextra.

[nothings]

Our general rule has been that it's not generally worth fixing stuff that only shows up with -Wextra, but that's because fixes aren't always straightforward and can even conflict. In this case, since there's a straightforward way to fix it, and given that it seems unlikely clang is going to change it's mind about warning for this, I would accept a PR fixing it.

[N-R-K]

Hi, I would suggest NOT using memset() and instead using s = { ZERO_INIT }; where ZERO_INIT is

#ifdef __cplusplus
    #define ZERO_INIT
#else    
    #define ZERO_INIT 0
#endif

In C this will end up being s = { 0 } which won't produce warning and in c++ it'll be s = { } which also doesn't seem to produce warning: https://godbolt.org/z/3ncYxb3sd

Additionally, I would suggest (when using C) to explicitly use -Wno-missing-field-initializers as it's braindead warning that catches more false positive than actual issues, in my experience of course :)

[nothings]

using -Wno-missing-field-initializers isn't an option because it's a header-file library so we don't control the compiler settings

[N-R-K]

@nothings Yes I am aware. That was a suggestion to the users rather. Implicit zero initialization is a pretty well defined behavior in C and from my experience that warning flag makes a lot of assumptions and thus is quite inconsistent. Consider the following example:

    struct test { int x; int y; int z; };
    struct test a = { 1, 2 };
    struct test b = { .x = 1, .y = 2 };
    struct test c = { .x = 0 };

It produces warning for a, but doesn't warn on b or c (https://godbolt.org/z/eq86qMP5b) But if you're trying to be C89 compliant (which seems to be the case here from what I read on the README) then you cannot use the 2nd version since designated-initializers is a C99 feature.

---

Anyhow, unless there's some case where the ZERO_INIT suggestion breaks in C++ (since it has alot more complex rules which I'm not aware of) then I think it should be a cleaner approach compared to doing memset().

[nothings]

I guess my feeling is that I'd favor memset() over ZERO_INIT just because it's (I think) the conventionally accepted solution, and it's also syntactically similar to how conventional wisdom is converging on using memcpy() for type aliasing instead of unions, so people should get it. But it's not a very strong feeling.

[N-R-K]

That's fine by me, I can update the PR to use memset() if that's what you prefer. I suggested the ZERO_INIT way since it puts the declaration and initialization in one place.

Don't have too strong of a preference, especially since I think it's the compiler warning that's at fault here, for being a false-positive.

---

One last thing, I don't think this matters in practice since systems with non-zero null pointer is pretty rare and it's not worth worrying about, but leaving it out here just for the sake of info: http://c-faq.com/null/machexamp.html.

TLDR: memset() to 0 won't be a valid null pointer on (rare) systems where null pointer isn't all bits zero, but = {0} will be.

[nothings]

TLDR: memset() to 0 won't be a valid null pointer on (rare) systems where null pointer isn't all bits zero, but = {0} will be.

Yep, this is why I learned never to use memset and to use 0-initialization instead decades ago. But, emoji shrug

[N-R-K]

Not sure if systems with non-zero null pointer would be the target audience for this library; but if we do care about dealing with that edge case correctly, there's two more options I can think of.

Do note, I'm not advocating for these, I think they're quite "ehh". But throwing it out anyways.

1. Explicitly initializing all the members.

   s = { 0, 0, { 0 }, 0 }; /* manually */
   s = { STBI_WRITE_CONTEXT_ZERO_INIT }; /* same as above, but via a macro */
   /* NOTE: this macro won't require any #ifdef __cplusplus since it'll init everything explicitly */

2. memset()-ing and then manually assigning the pointers to null

   memset(&s, 0, sizeof(s));
   s.func = 0;
   s.context = 0;

[nothings]

could also just have a style thing of 'don't rely on 0-memset to init pointers, but don't bother explicitly nulling them either, just make sure they get written'

[nothings]

maybe the best would just be to give each structure type its own "zero" function which memsets it and then explicitly sets the pointers to 0. if there are nested structs they can call each other.

[nothings]

Although at that point I'm starting to think about ZERO_INIT being saner.

Maybe something like this would be clearer:

#ifdef __cplusplus
#define stbi_empty_if_cpp(x) 
#else
#define stbi_empty_if_cpp(x)  x
#endif

Foo s = { stbi_empty_if_cpp(0) };

(btw, the reason I moved this discussion here outside of the PR was so you wouldn't change the PR until we decided. Whoops.)

I don't know if that's actually going to be easier for someone unfamiliar with the code to comprehend than ZERO_INIT, which is probably more self-explanatory despite the above approach being meant to be more explicit about the mechanism.

[nothings]

This might be clearer, and in theory could be used for other things.

Foo s = STBI_C_OR_CPP( {0}, {} );

(Can't put this macro inside the {} because you'd need an empty argument for the C++ case, and ANSI C doesn't allow empty macro arguments IIRC.)

[N-R-K]

(btw, the reason I moved this discussion here outside of the PR was so you wouldn't change the PR until we decided. Whoops.)

Ahh, no worries. I interpreted https://github.com/nothings/stb/issues/1099#issuecomment-1066118311 as a confirmation. I'll wait for you to explicitly tell me to update the PR next time before doing so.

This might be clearer, and in theory could be used for other things.

Foo s = STBI_C_OR_CPP( {0}, {} );

This looks fine to me, and yeah, I can see it being useful on some other places as well compared to the ZERO_INIT macro which would be helpful only for struct/array init.

Thinking a bit more, this is probably the best suggestion so far; it's re-useable, avoids the non-zero nullptr issue (for what that's worth) and doesn't look as bad/ugly as some of the other suggestions (IMO).

[Sokolmish]

In older versions of gcc, using {0} initializer produces warning for the C code as well. Here is example for gcc 4.6: https://godbolt.org/z/83Wv4oYx8.

[N-R-K]

In older versions of gcc, using {0} initializer produces warning for the C code as well. Here is example for gcc 4.6: https://godbolt.org/z/83Wv4oYx8.

Yes, afaik they later changed it to not produce warnings on such cases since it was catching too many false positive than actual issues. And this is basically why I don't like that flag, and what I meant in https://github.com/nothings/stb/issues/1099#issuecomment-1017432049 when I said this flag makes a lot of assumptions.

[nothings]

The old gcc thing tempts me to just punt entirely. But since Clang is unlikely to change their behavior, it's still probably better to fix it for everything except old gcc, which can remain broken.

[martinlicht]

I observe the same with both GCC and Clang whenever -Wextra is activated. Maybe you can insert a comment in the source how to fix it locally?