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

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

Closed
3 tasks done
akien-mga opened this issue Feb 15, 2022 · 9 comments
Closed
3 tasks done

Comments

@akien-mga
Copy link
Member

akien-mga commented Feb 15, 2022

Godot version

4.0.dev (98b97d3)

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.

  • Chunk A:
core/object/script_language.cpp
editor/action_map_editor.cpp
editor/animation_bezier_editor.cpp
editor/animation_track_editor.cpp
editor/audio_stream_preview.cpp
editor/code_editor.cpp
editor/connections_dialog.cpp
editor/create_dialog.cpp
editor/debugger/debug_adapter/debug_adapter_server.cpp
editor/debugger/editor_debugger_inspector.cpp
editor/debugger/editor_debugger_node.cpp
editor/debugger/editor_debugger_tree.cpp
editor/debugger/editor_network_profiler.cpp
editor/debugger/editor_profiler.cpp
editor/debugger/editor_visual_profiler.cpp
editor/debugger/script_editor_debugger.cpp
editor/editor_about.cpp
editor/editor_audio_buses.cpp
editor/editor_autoload_settings.cpp
editor/editor_dir_dialog.cpp
editor/editor_export.cpp
editor/editor_feature_profile.cpp
editor/editor_file_dialog.cpp
editor/editor_file_system.cpp
editor/editor_help.cpp
editor/editor_help_search.cpp
editor/editor_inspector.cpp
editor/editor_log.cpp
editor/editor_node.cpp
editor/editor_path.cpp
editor/editor_plugin.cpp
editor/editor_plugin_settings.cpp
editor/editor_properties_array_dict.cpp
editor/editor_properties.cpp
editor/editor_resource_picker.cpp
editor/editor_run_native.cpp
editor/editor_settings_dialog.cpp
editor/editor_spin_slider.cpp
editor/editor_toaster.cpp
editor/editor_zoom_widget.cpp
editor/export_template_manager.cpp
editor/filesystem_dock.cpp
editor/find_in_files.cpp
  • Chunk B:
editor/groups_editor.cpp
editor/import_defaults_editor.cpp
editor/import_dock.cpp
editor/import/dynamic_font_import_settings.cpp
editor/import/scene_import_settings.cpp
editor/inspector_dock.cpp
editor/localization_editor.cpp
editor/node_dock.cpp
editor/plugin_config_dialog.cpp
editor/plugins/abstract_polygon_2d_editor.cpp
editor/plugins/animation_blend_space_1d_editor.cpp
editor/plugins/animation_blend_space_2d_editor.cpp
editor/plugins/animation_blend_tree_editor_plugin.cpp
editor/plugins/animation_player_editor_plugin.cpp
editor/plugins/animation_state_machine_editor.cpp
editor/plugins/animation_tree_editor_plugin.cpp
editor/plugins/asset_library_editor_plugin.cpp
editor/plugins/audio_stream_editor_plugin.cpp
editor/plugins/canvas_item_editor_plugin.cpp
editor/plugins/collision_shape_2d_editor_plugin.cpp
editor/plugins/control_editor_plugin.cpp
editor/plugins/cpu_particles_2d_editor_plugin.cpp
editor/plugins/cpu_particles_3d_editor_plugin.cpp
editor/plugins/curve_editor_plugin.cpp
editor/plugins/debugger_editor_plugin.cpp
editor/plugins/font_editor_plugin.cpp
editor/plugins/gpu_particles_2d_editor_plugin.cpp
editor/plugins/gpu_particles_3d_editor_plugin.cpp
editor/plugins/gpu_particles_collision_sdf_editor_plugin.cpp
editor/plugins/gradient_editor_plugin.cpp
editor/plugins/material_editor_plugin.cpp
editor/plugins/mesh_editor_plugin.cpp
editor/plugins/node_3d_editor_plugin.cpp
editor/plugins/ot_features_plugin.cpp
editor/plugins/path_2d_editor_plugin.cpp
editor/plugins/path_3d_editor_plugin.cpp
editor/plugins/polygon_2d_editor_plugin.cpp
editor/plugins/polygon_3d_editor_plugin.cpp
editor/plugins/replication_editor_plugin.cpp
editor/plugins/resource_preloader_editor_plugin.cpp
editor/plugins/root_motion_editor_plugin.cpp
editor/plugins/script_editor_plugin.cpp
editor/plugins/script_text_editor.cpp
  • Chunk C:
editor/plugins/shader_editor_plugin.cpp
editor/plugins/shader_file_editor_plugin.cpp
editor/plugins/skeleton_3d_editor_plugin.cpp
editor/plugins/sprite_frames_editor_plugin.cpp
editor/plugins/text_control_editor_plugin.cpp
editor/plugins/texture_3d_editor_plugin.cpp
editor/plugins/texture_editor_plugin.cpp
editor/plugins/texture_layered_editor_plugin.cpp
editor/plugins/texture_region_editor_plugin.cpp
editor/plugins/theme_editor_plugin.cpp
editor/plugins/theme_editor_preview.cpp
editor/plugins/tiles/tile_atlas_view.cpp
editor/plugins/tiles/tile_data_editors.cpp
editor/plugins/tiles/tile_map_editor.cpp
editor/plugins/tiles/tiles_editor_plugin.cpp
editor/plugins/tiles/tile_set_atlas_source_editor.cpp
editor/plugins/tiles/tile_set_editor.cpp
editor/plugins/tiles/tile_set_scenes_collection_source_editor.cpp
editor/plugins/visual_shader_editor_plugin.cpp
editor/plugins/voxel_gi_editor_plugin.cpp
editor/progress_dialog.cpp
editor/project_export.cpp
editor/project_manager.cpp
editor/project_settings_editor.cpp
editor/property_editor.cpp
editor/property_selector.cpp
editor/quick_open.cpp
editor/reparent_dialog.cpp
editor/scene_tree_dock.cpp
editor/scene_tree_editor.cpp
editor/script_create_dialog.cpp
editor/shader_create_dialog.cpp
editor/shader_globals_editor.cpp
modules/csg/csg_shape.cpp
modules/gdnative/gdnative_library_singleton_editor.cpp
modules/gdnative/nativescript/nativescript.cpp
modules/gdscript/language_server/gdscript_language_server.cpp
modules/gridmap/grid_map.cpp
modules/gridmap/grid_map_editor_plugin.cpp
modules/navigation/navigation_mesh_editor_plugin.cpp
modules/visual_script/editor/visual_script_editor.cpp
modules/visual_script/editor/visual_script_property_selector.cpp

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
Copy link
Contributor

jmb462 commented Feb 15, 2022

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

@jmb462
Copy link
Contributor

jmb462 commented Feb 15, 2022

@akien-mga Should we remove empty default case ?

default:
       break;

@akien-mga
Copy link
Member Author

@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
Copy link
Contributor

I can do chunk B

@jakobbouchard
Copy link
Contributor

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
Copy link
Member Author

akien-mga commented Feb 16, 2022

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
Copy link
Contributor

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
Copy link
Member Author

[[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
Copy link
Member Author

All done, thanks @jmb462 @megalobyte @jakobbouchard!

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

No branches or pull requests

4 participants