Skip to content

fix: fix build warning in switch case (#118) #119

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

Merged
merged 3 commits into from
Jun 19, 2025

Conversation

MisterRaindrop
Copy link
Contributor

fix some compilation warnings. see issue (#118)

@mapleFU
Copy link
Member

mapleFU commented Jun 6, 2025

can we do like below to avoid default in switch case?

switch () {
 ...
}
+ Unreachable();

@MisterRaindrop
Copy link
Contributor Author

If there is no break statement in a certain case, the program may execute the Unreachable() function, which is not what we want (what we want is for each case to handle the situation and then exit, so we must be careful with the break statements). Additionally, when the compiler does not support exhaustive checking, we cannot know at compile - time if there are any missing cases.

Of course, if it is indeed placed later, I can also make modifications.

@MisterRaindrop
Copy link
Contributor Author

Which way do you think is better?

switch () {
 ...
}
+ Unreachable();

or

switch () {
 ...
default:
+ Unreachable();
}

@mapleFU
Copy link
Member

mapleFU commented Jun 6, 2025

Isn't all this pattern in this case is switch (...) { case1: return case2: return ...}? If a new enum is added, without default, compiler would compile error here

@MisterRaindrop
Copy link
Contributor Author

Okay. If you’re sure it will cause an error, then I’ll modify it to put it outside.

Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

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

LGTM

@zhjwpku
Copy link
Collaborator

zhjwpku commented Jun 17, 2025

@Fokko @Xuanwo I think this is a valid fix. Could you take a look?

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Nice, thanks for fixing this @MisterRaindrop

@Fokko Fokko merged commit db8aa74 into apache:main Jun 19, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants