Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Replace ERR_FAIL_COND with ERR_FAIL_NULL where applicable #81487

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Sep 9, 2023

The primary reason is plain correctness, but several of these cases, especially the wider cases that will be added soon, are hard to tell what they should be with the current code, so having an explicit null check makes reading the code easier

The most straight-forward argument, taken from display_server_web.cpp:

ERR_FAIL_COND_V_MSG(!tts, false, "Enable the \"audio/general/text_to_speech\" project setting to use text-to-speech.");

And the one from display_server_windows.cpp:

ERR_FAIL_COND_V_MSG(!tts, false, "Enable the \"audio/general/text_to_speech\" project setting to use text-to-speech.");

Only one is a null check, despite being entirely identical otherwise.

Edit: Also cleaned up some error messages for style when they were already part of the change.

@AThousandShips
Copy link
Member Author

Going through various areas, can make a single massive PR for all the areas or split it into dedicated parts, will start by adding the individual commits to this and will split if required

@AThousandShips AThousandShips requested review from a team as code owners September 9, 2023 16:57
@AThousandShips AThousandShips changed the title [Core] Replace ERR_FAIL_COND with ERR_FAIL_NULL where applicable Replace ERR_FAIL_COND with ERR_FAIL_NULL where applicable Sep 9, 2023
@timothyqiu
Copy link
Member

I think the advantage of ERR_FAIL_NULL over ERR_FAIL_COND is its clear error message.

But this advantage does not exist for ERR_FAIL_NULL_MSG relative to ERR_FAIL_COND_MSG as they are basically the same except that what the parameter means is opposite.

@AThousandShips
Copy link
Member Author

The advantage is communication, it makes it clear what it checks for, see the case in the OP

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from a quick review. If I understand correctly, this won't compile if it's incorrect, so I think this is pretty safe.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 11, 2023

Did some changes that I rolled back before which didn't show up until running (they were is_null cases) cases with but these are all pointer based, as Variant, booleans, and integers all break on compile

@AThousandShips
Copy link
Member Author

The remaining parts of the engine have the following sizes in changes:

  • Scene + main: 23
  • Platform: 83
  • Drivers: 342
  • Editor: 218
  • Modules: 545
  • Servers: 1,104

Can combine as desired, the scan is the same so same considerations apply, servers is large, but also largely a simple pattern

All were applied with a filter in Kate, and hand picked based on the names, and rolled back a few cases that were variant or bool

@akien-mga akien-mga merged commit cc61c9d into godotengine:master Sep 12, 2023
15 checks passed
@AThousandShips AThousandShips deleted the null_check_core branch September 12, 2023 10:10
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants