Ensure members are initialized properly

[asmaloney]

(PR for 3.2 here.)

[akien-mga]

Can you explain the rationale?

There are hundreds of classes with members which are not default-initialized, but are also not used uninitialized as the class' logic always ensures that they're initialized before being used.

If we think that everything should be default initialized (ah, if only that was a feature of C++ compilers...), this should be done as a coordinated effort and possibly with benchmarking of the potential performance impact, and not as individual PRs for every single class in the codebase.

[asmaloney]

If we think that everything should be default initialized

I do :)

if only that was a feature of C++ compilers

Agreed!

I'm coming (mainly) from outside the game-dev world where it is considered good practice to always have data in a consistent state. Over the decades I've dealt with many, many issues that resulted from uninitialized data. (In fact I'm looking into one right now.)

1. Relying on other logic in the class is fragile. It's too easy to get wrong, to miss, or to break accidentally. Unless everyone working on the code base is disciplined and takes the time to read and understand all of the class's code, someome will eventually introduce a way to use the data uninitialized. This is going to be exacerbated as more developers come on board.

2. Problems with uninitialized data can be very difficult to diagnose because you end up with what seems like random behaviour. "It worked this time" - "oh, nope it doesn't work". This can be very time consuming to track down & fix and is super frustrating for the user.

3. Initializing is explicit and in one place (hopefully the header these days) rather than spread throughout the code.

4. It can give an idea of what magnitude a valid value looks like. I don't have an example from the engine yet, but if you have a member var length and it's initialized to say 400, then you know what magnitude you're dealing with. (I admit this is a weaker argument, but I have seen plenty of cases where this is useful when reading code or trying to track down a problem - should this be 0.4 or 40.0?)

5. I understand that in some critical places it might make sense not to do this, but with class members that is very much the exception (and would be commented as such).

6. This PR fixes at least a couple of cases where one of these members can be used without being initialized. Granted these are varying degrees of "unlikely scenario", but the probability is not zero because people make mistakes.

7. I'm happy to play "bit-janitor" and work on this if anything I've said is convincing :-)

[Xrayez]

I can testify the uninitialized members issues and various heisenbugs throughout Godot, I've personally caught a few of them: #31604, #24283 (interestingly, all caught with the unit testing framework in place, and would probably left unnoticed without this). But yeah it would be better if this could be done on the entire codebase (?).

But all or nothing doesn't quite work here IMO. I know that many programmers rely on logic and rationale, but issues like this demand intuition. And I feel like this particular PR could likely solve some problems already (mainly because I've stumbled upon some serialization issues already).

To note, there was a major refactor regarding porting member default initialization from constructor to declaration #38697, but many places are still left uninitialized, like this:

godot/core/message_queue.h

Lines 65 to 68 in 1f6f364

	uint8_t *buffer;
	uint32_t buffer_end = 0;
	uint32_t buffer_max_used = 0;
	uint32_t buffer_size;

This sort of changes remind me of wanting to add some simple comments to document some parts of the engine internals, but then realizing that those changes would "unnecessarily pollute the git history" if done via dedicated PR... The coordinate effort doesn't work either because that's on the case-to-case basis. I strongly believe that history should not be clean for the sake of it.

[asmaloney]

If the project were to do this for the whole codebase (and I think it should - I can't think of any C++ project I've worked with/on in the last 25 years that doesn't initialize members by default - see reasoning above), it is not practical to do it all at once.

If I were approaching it, I would first change the "dev docs" to say that all class and struct members should be initialized, then I would make the changes in a series of commits organized as much as possible around the filesystem structure. So directory-by-directory, trying to keep the PRs at a reasonable size to be reviewed quickly.

I would probably have a standard commit message - something like "{init} Initialize members in [dir]". This would make it obvious it's part of this "project" and make them easier/faster to identify and review.

These PRs would do 2 things:

• ensure all members are initialized
• move any initialization to the declaration when possible (already done for the most part in master I think)

Due to time and effort, I would only do this on the master branch rather than trying to do it on both 3.2 and 4.0.

Again - I'm willing to spend the time on this if the powers-that-be approve.

[aaronfranke]

@asmaloney Is this still desired? If so, it needs to be rebased and tested on the latest master branch.

[asmaloney]

It seems these have since been fixed in other PRs.

It looks like the approach I mention to doing this systemically by folder is being done with #43636, though I think the "code style guidelines" still needs to be updated to say that all class and struct members should be initialized.

[asmaloney]

(Actually, it looks like that issue is only covering what cppcheck finds, but it's better than nothing I suppose.)