Skip to content

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
lishuxu pushed a commit to lishuxu/iceberg-cpp that referenced this pull request Jul 5, 2025
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