godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.71k stars 20.12k forks source link

Port existing `_notification` code to use `switch` statements instead of `if` #58155

Closed akien-mga closed 2 years ago

akien-mga commented 2 years ago

Godot version

4.0.dev (98b97d34df7f3ab2cafc3847358c231c3d357b40)

Issue description

Following #58151, the rest of the codebase should be given the same treatment to review all _notification methods and ensure that they use switch statements instead of a list of if (p_what == NOTIFICATION_*).

See the above PR for details of what the preferred style should be like.

58151 handled all code in scene, so what remains is the following. I split it in 3 chunks of approximately similar size.

If you want to work on this issue, please comment to indicate which chunk you're working on to avoid having multiple contributors do the same chunks.

Note: Those are the list of files with the string ::notification(, some of them might already be using switch statements and just need a quick touch-up (adding separation lines if missing, ensuring they use code blocks and break;).

jmb462 commented 2 years ago

I can take chunk A if it's OK for you.

jmb462 commented 2 years ago

@akien-mga Should we remove empty default case ?

default:
       break;
akien-mga commented 2 years ago

@akien-mga Should we remove empty default case ?

default: break;

Yeah I think so. I don't see any reason to have them in _notification switches.

megalobyte commented 2 years ago

I can do chunk B

jakobbouchard commented 2 years ago

Never touched C++ before, but I want to learn it. I can do Chunk C, unless you'd prefer people who already used C++.

akien-mga commented 2 years ago

Feel free to give it a try, it can be a good way to learn some syntax. Don't hesitate to join the Godot Chat if you have questions on how to handle some parts.

Depending on the situations, it might require some understanding of how C++ switch statement works, notably with regard to implicit fallthrough and the need to break; to prevent fallthrough, or make it explicit with [[fallthrough]];. You also need to make sure to follow the style guidelines outlined in #58151.

jakobbouchard commented 2 years ago

I've used switch statements before, I assume that the implicit fallthrough would be like the first case in the example in #58151? I'm familiar with break;, not with [[fallthrough]];, but I assume it does just that, but is more "verbose". Unless it differs in the way it behaves? I just started working on it about 10 minutes ago, just so you know.

akien-mga commented 2 years ago

[[fallthrough]]; lets the compiler (and the reader) know that the fallthrough (which is a language feature) was actually intentional. Unintentional fallthroughs are the cause of many bugs in C/C++ codebases so compilers now raise warnings (with -Wimplicit-fallthrough) when this happens without an explicit [[fallthrough]]; attribute.

akien-mga commented 2 years ago

All done, thanks @jmb462 @megalobyte @jakobbouchard!