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

Use switch consistently in _notification (scene folder) #58151

Merged
merged 1 commit into from Feb 15, 2022

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Feb 15, 2022

Unifies the code style in _notification methods by using switch statements consistently, with the following opinionated style (which mostly matches our current "best practice", at least it's what recently added code tends to use):

void MyClass::_notification(int p_what) {
	switch (p_what) {
		case NOTIFICATION_ENTER_TREE:
		case NOTIFICATION_MOVED_IN_PARENT: {
			// Do stuff.
		} break;

		case NOTIFICATION_EXIT_TREE: {
			// Do other stuff.
		} break;

		case NOTIFICATION_FOCUS_ENTER: {
			// Do custom stuff for focus enter.
			[[fallthrough]];
		}
		case NOTIFICATION_FOCUS_EXIT: {
			// Do common stuff for any focus change.
		} break;
}

I.e.:

  • Always use brackets for case blocks
  • Always use break; after the closing bracket
  • Keep a separation line between cases, unless they're a fallthrough (instead of using a weird empty line before } break;)
    • This is not a policy for all switch statements in the codebase, only for _notification ones which tend to be very big. (Can still be used for other switches as needed when more visual separation helps readability.)

Reviewing the whole of scene was a pretty tedious 2.5 hours of work, I might solicit contributors help to port the rest of the codebase if we agree about those changes.

This is best reviewed with whitespace changes hidden (add ?w=1 to PR files view URL): https://github.com/godotengine/godot/pull/58151/files?w=1

@akien-mga akien-mga added this to the 4.0 milestone Feb 15, 2022
@akien-mga akien-mga requested review from a team as code owners February 15, 2022 17:19
@akien-mga akien-mga force-pushed the notification-switch-scene branch 2 times, most recently from 7719fc1 to c1cacda Compare February 15, 2022 17:32
@akien-mga
Copy link
Member Author

akien-mga commented Feb 15, 2022

Note about [[fallthrough]]: IMO it would look better if it could be put in the place of break;, like this:

		case NOTIFICATION_FOCUS_ENTER: {
			// Do custom stuff for focus enter.
		} [[fallthrough]];
		case NOTIFICATION_FOCUS_EXIT: {
			// Do common stuff for any focus change.
		} break;

But IIRC this didn't work at least for some compilers as the attribute is expected to qualify a null statement, which the { } code block isn't.
It might work with:

		case NOTIFICATION_FOCUS_ENTER: {
			// Do custom stuff for focus enter.
		}; [[fallthrough]];
		case NOTIFICATION_FOCUS_EXIT: {
			// Do common stuff for any focus change.
		} break;

(added semicolon)
or:

		case NOTIFICATION_FOCUS_ENTER: {
			// Do custom stuff for focus enter.
		}
		[[fallthrough]];
		case NOTIFICATION_FOCUS_EXIT: {
			// Do common stuff for any focus change.
		} break;

But I'll experiment with those in a separate PR.


Edit:

Well } [[fallthrough]]; seems to work fine with latest GCC and Clang at least, tested on this use case with:

class SomeClass {
public:
	enum SomeEnum {
		A,
		B,
	};

	int handle_a(SomeEnum a) {
		int b = 0;
		switch (a) {
			case A: {
				b = 42;
			} [[fallthrough]];
			case B:
				b = 12;
				break;
			default:
				b = 128;
				break;
		}
		return b;
	}
};

int main() {
	SomeClass c;
	return c.handle_a(SomeClass::A);
}

I'll change these in a follow-up PR throughout the codebase.


Edit 2: Now I remember why we can't use this code style. clang-format messes it up:

diff --git a/core/io/http_client_tcp.cpp b/core/io/http_client_tcp.cpp
index 23381257e5..c5550a29df 100644
--- a/core/io/http_client_tcp.cpp
+++ b/core/io/http_client_tcp.cpp
@@ -132,7 +132,8 @@ static bool _check_request_url(HTTPClientTCP::Method p_method, const String &p_u
 			if (p_url == "*") {
 				return true;
 			}
-		} [[fallthrough]];
+		}
+			[[fallthrough]];
 		default:
 			// Absolute path or absolute URL.
 			return p_url.begins_with("/") || p_url.begins_with("http://") || p_url.begins_with("https://");

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Long overdue!

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.

None yet

2 participants