Fix union order to simplify empty initializers #101560
Conversation
|
I strongly advise this PR to be backported to earlier versions if the unions exist; since Valgrind shows how these uninitialized values propagate all way down to the Vulkan driver. The problem may be worse than we think (or it could be harmless). |
This is a followup to PR godotengine#101344 (commit 0e06eb8). Some of them were not an issue because Godot was initializing all members, but they were "fixed" just in case since it could become a problem in the future. Valgrind was specifically complaining about HashMapData & GlobalPipelineData.
darksylinc
force-pushed
the
matias-union-order
branch
from
0bf42f1 to
7b2f1e1
Compare
| @@ -250,6 +250,10 @@ class RasterizerSceneGLES3 : public RendererSceneRender { | |||
| }; | |||
|
|
|||
| union { | |||
| struct { | |||
| uint64_t sort_key1; | |||
There was a problem hiding this comment.
Will this initialize both keys, or just sort_key_1?
There was a problem hiding this comment.
As far as I understand it will initialize both keys:
union initialization where only the very first element of the union is picked when using empty initializers such as {} if no other element in the union has an explicit initializer.
In this case the nameless struct is the first element in the union.
There was a problem hiding this comment.
Ahhh okay. I misunderstood what was causing the problem. I thought the first element of the struct was being grabbed, but no. The problem in these cases is that the first struct was smaller than the second.
So a simple rule of thumb (that this PR enforces) is to always put the larger element first in a union
akien-mga
added
cherrypick:4.2
|
I checked and some of these unions exist in |
|
Thanks! |
This is a followup to PR #101344 (commit 0e06eb8).
Some of them were not an issue because Godot was initializing all members, but they were "fixed" just in case since it could become a problem in the future.
Valgrind was specifically complaining about HashMapData & GlobalPipelineData.