Fully initialize all members of structs IdentifierActions, GeneratedCode and DefaultIdentifierActions

[Chubercik]

Fixes #88015.

This one simple change

- ShaderCompiler::DefaultIdentifierActions actions;
+ ShaderCompiler::DefaultIdentifierActions actions = {};

is enough for Godot to compile on my setup, however, @akien-mga pointed out that this might be an issue in other places in the code here. He proposed an alternative fix (changing the struct code to fully initialize all its members), but I'd like to wait for the rendering team to voice their opinions about this.

EDIT: I went ahead with the alternative fix (and then some more for completeness' sake).

[akien-mga]

Yeah I think you might have similar warnings in the other places if you do a clean build. At any rate, it doesn't make much sense IMO to initialize it only here and not everywhere else. So I think changing the struct to make sure it's always fully initialized would be the best option.

WDYT @clayjohn @BastiaanOlij ?

[AThousandShips]

I think the issue might be the two texture modes in this struct

[Chubercik]

So

ShaderLanguage::TextureFilter default_filter = ShaderLanguage::TextureFilter::FILTER_DEFAULT;
ShaderLanguage::TextureRepeat default_repeat = ShaderLanguage::TextureRepeat::REPEAT_DEFAULT;

should fix the issue?

Edit: This is the bare minimum to fix the issue at hand, more initializations were added in the PR for completeness' sake.

[AThousandShips]

I'd suggest initialising the values in the IdentifierActions struct as well while at it, like the uniforms pointer there and the booleans

[Chubercik]

Think I'm gonna need some help here - I assume uniforms should be initialized to nullptr, given that it's a pointer, but what about the other values you speak of: they're of type HashMap, not sure what the syntax would look like here..

[AThousandShips]

Not those, the bool values, the maps should be untouched they have default initialization

[Chubercik]

Are we talking about the same bools?

HashMap<StringName, Pair<int *, int>> render_mode_values;
HashMap<StringName, bool *> render_mode_flags;
HashMap<StringName, bool *> usage_flag_pointers;
HashMap<StringName, bool *> write_flag_pointers;

I considered these lines of code, but the bool (and one int) pointers are inside HashMaps and I'm not sure what would initialization of those alone look like.

bool uses_global_textures;
bool uses_fragment_time;
bool uses_vertex_time;
bool uses_screen_texture_mipmaps;
bool uses_screen_texture;
bool uses_depth_texture;
bool uses_normal_roughness_texture;

If you were talking about these bools, they're in a different struct 😅

[AThousandShips]

Yes I was talking about the GeneratedCode my bad :)

[Chubercik]

That's fine, one more question:

struct Texture {
	StringName name;
	ShaderLanguage::DataType type;
	ShaderLanguage::ShaderNode::Uniform::Hint hint;
	bool use_color = false;
	ShaderLanguage::TextureFilter filter;
	ShaderLanguage::TextureRepeat repeat;
	bool global;
	int array_size;
};

This snippet is from within GeneratedCode, should I also initialize type, hint, filter, etc. here?

[AThousandShips]

I'd say yes, prefer to have everything basically intialized, false for bool, 0 for int and what ever default enum value applies

[clayjohn]

Yeah I think you might have similar warnings in the other places if you do a clean build. At any rate, it doesn't make much sense IMO to initialize it only here and not everywhere else. So I think changing the struct to make sure it's always fully initialized would be the best option.

WDYT @clayjohn @BastiaanOlij ?

This case is a bit unique. I think it is fully initialized in other areas property-by-property. The difference is that here we don't care about the actions so we left them all empty.

[Chubercik]

Hope I picked sane defaults for enums.

[BastiaanOlij]

Yeah I think you might have similar warnings in the other places if you do a clean build. At any rate, it doesn't make much sense IMO to initialize it only here and not everywhere else. So I think changing the struct to make sure it's always fully initialized would be the best option.

WDYT @clayjohn @BastiaanOlij ?

I'm not against it, I've ran into problems before with compiler difference where some zero out structs and others keep junk data in there and caused sudden crashes. So if we're checking on that with compiler checks, might as well. I think the only issue with defining defaults in the structs is that there may be confusion when structs come from 3rd party APIs (like Vulkan and OpenXR). But we don't really have control over that.

[RandomShaper]

I'd say yes, prefer to have everything basically intialized, false for bool, 0 for int and what ever default enum value applies

I agree and would like to add that, when the default value for an enum type is not relevant (you just want to avoid garbage) or when it's clear (somewhat fuzzy, I know) what's the natual default, I'd also find = {} acceptable.

Regarding 3rd-part structs, in the rendering driver code, for instance, I've chosen to do = {} on every Vulkan and D3D12 structs, to ensure C-like APIs don't provide garbage data.

[akien-mga]

Looks good to me.

Rendering headers are some of the last ones where we don't systematically initialize every struct members, and they have a lot of structs. So I'd like someone from the rendering team to sign off on this before merging, just in case I'm missing good reasons to keep those uninitialized.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.2.2.