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

VerifyWidget: Handle Severity::None case in Verify() #8382

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@tkln
Copy link
Contributor

tkln commented Oct 5, 2019

The case body is empty because VolumeVerifier doesn't actually report
problems with severity of None.

Fixes "warning: enumeration value ‘None’ not handled in switch [-Wswitch]"
warning reported by gcc.

The case body is empty because VolumeVerifier doesn't actually report
problems with severity of None.

Fixes "warning: enumeration value ‘None’ not handled in switch [-Wswitch]"
warning reported by gcc.
@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Oct 5, 2019

I wonder if we should instead add a default case that does something like severity = std::to_string(problem.severity); just to future-proof things a little...

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Oct 5, 2019

I would go further and put an assertion/critical error there so if it ever happens, it explodes loudly.

@tkln

This comment has been minimized.

Copy link
Contributor Author

tkln commented Oct 5, 2019

@CookiePLMonster I agree that it's often a good idea to have a default case where all unhandled values fall into and signal errors. However, in this case it seems a bit redundant since problem.severity is a strongly typed enum class which makes unhandled values detectable at compile time. I'm not sure if "future-proofing" like that is a good idea because a default case will mask those compiler warnings.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Oct 7, 2019

Indeed... can this warning be upgraded to an error only in this place maybe? I think that would be best, as then it would explode - but at compile time.

@stenzek
stenzek approved these changes Nov 8, 2019
@stenzek stenzek merged commit 422c3f7 into dolphin-emu:master Nov 8, 2019
8 checks passed
8 checks passed
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.