libsdl-org / SDL_mixer

An audio mixer that supports various file formats for Simple Directmedia Layer.
zlib License
434 stars 146 forks source link

Remove all initializers from static loader arrays #355

Closed Wohlstand closed 2 years ago

Wohlstand commented 2 years ago

Addition to https://github.com/libsdl-org/SDL_mixer/commit/143b0bbfd50654af667c87143c1a2d69ef80f151

EDIT: As was discussed below, I changed the entire goal of this PR:

C standard gives guarantee they will be NULLed by default: https://port70.net/~nsz/c/c89/c89-draft.html#3.5.7

If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant. If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate.

slouken commented 2 years ago

Actually let's just remove all the static initializers. That way if we change the decoder structures they'll still be zeroed with no additional work.

Wohlstand commented 2 years ago

Actually let's just remove all the static initializers. That way if we change the decoder structures they'll still be zeroed with no additional work.

Do you mean I should remove these initializers and just add the SDL_memset(.., 0, sizeof(..) at beginning of every initializer?

Also, that needs also make the "loaded" field being off the structure and being statically initialized as 0, otherwise, if structure is not initialized, then, garbage will make the illusion like library was been initialized, so, make the sort of this:

изображение


The fact: I reminded some other but similar case: Before you did major rework at mainstream Mixer, I had developed my own variant of improvement and I had a different way, so, lemme find and show you some (maybe revive that my concept to here?)

It's about the Mix_MusicInterface initializer, and I had it being dynamically initialized in the next way:

EDIT: I found an exact example: https://github.com/WohlSoft/SDL-Mixer-X/blob/sdl-mixer-x-1-legacy/src/codecs/music_flac.c#L118-L158, but seems, need to make something simpler than this.

изображение

slouken commented 2 years ago

No, I mean:

static fluidsynth_loader fluidsynth;

All the fields are initialized to 0 and NULL by C language definition. We don't need to provide initializers unless we want non-zero values.

Wohlstand commented 2 years ago

All the fields are initialized to 0 and NULL by C language definition. We don't need to provide initializers unless we want non-zero values.

Very depends on the compiler, because I literally had these structures being filled with garbage. The full zeroing works on debug mode only, on release mode that will just a ton of garbage.

slouken commented 2 years ago

They were only filled with garbage because they were partially initialized. If you leave off the initializers entirely, you'll see that they are zeroed.

Wohlstand commented 2 years ago

Anyway, I want to make the small check:

Wohlstand commented 2 years ago

Anyway, I tried to search around, and people say that if you leave the structure field being just defined, it will be uninitialized at all. Where is exactly described that if you define the static structure field and being untouched, it will be automatically initialized by zeroes?

Wohlstand commented 2 years ago

btw, I found something here: https://coderedirect.com/questions/191022/are-the-members-of-a-global-structure-initialized-to-zero-by-default-in-c

The C99 / 2003 was mentioned, but what about C89/C90 ?

EDIT: https://port70.net/~nsz/c/c89/c89-draft.html#3.5.7

Wohlstand commented 2 years ago

Okay, I found some:

If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant. If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate.65

Wohlstand commented 2 years ago

@slouken, so, I triple-checked all stuff, and I updated this PR, so, should be fine now :eyes:

slouken commented 2 years ago

Looks good, thanks!

sezero commented 2 years ago

Oh yes, finally.

@slouken: Merge?

Wohlstand commented 2 years ago

@slouken: Merge?

Already merged :smirk_cat:

sezero commented 2 years ago

Already merged smirk_cat

Damn, missed it :)