Fix warning C26818 about switch statements needing default cases
#4715
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also remove an unnecessary, inconsistent comment.
These are all covered by the test. Cite the Standard to explain how we've handled all valid values.
Add a comment to explain why we've handled all cases.
These are all covered by the test. They're all used, i.e. making them `abort()` causes tests to fail.
These are both covered by the new test. The empty `default: break;` is used, i.e. making it `abort()` causes tests to fail. Add a comment to explain why `_Parse_range_specs()` guarantees that the other can't happen.
This is covered by the new test. The empty `default: break;` is used, i.e. making it `abort()` causes tests to fail.
This is covered by the new test. The empty `default: break;` is used, i.e. making it `abort()` causes tests to fail.
|
Sure, thanks @AlexGuteniev! 😻 Pushed a commit. |
|
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
barcharcraz
approved these changes
Jun 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes DevCom-10670613 VSO-2081258 / AB#2081258.
In general, we haven't attempted to be clean with respect to Core Guidelines warnings, as many of them are noisy and/or inappropriate for Standard Library code. However, warning C26818 "Switch statement does not cover all cases. Consider adding a 'default' label (es.79)." aligns with our conventions. We always like to have a
defaultas a reminder to think about what happens when none of thecases are selected (this is one of the relatively few scenarios where we like to write code that isn't necessary). The only exception to this convention is when we're switching on an enum and we've handled all of the enumerators (it can still be okay to have a "can't happen"default, but sometimes we don't bother), which fortunately this warning doesn't complain about.I added test coverage for all of these fixes, i.e. removing any fix will cause
GH_002094_cpp_core_guidelinesto fail.All of the empty
default: break;cases being added are used, i.e. making themabort()will cause other tests to fail.In
<format>, we have a couple of "can't happen" cases, where I've added_STL_UNREACHABLEwith comments explaining why.In
<xlocmon>, we havedefaultcases that can only be activated by users giving us a bogusmoney_base::pattern, so I've added_STL_ASSERT(false, "message")citing the same Standardese each time.While auditing all of our
switchstatements (which is how I found half of these), I found a few cases in old code where we weren't obeying our syntax convention: aswitch's lastcaseordefaultmust alwaysbreak. Falling off of the end is an unacceptable maintenance hazard.Finally, I removed an unnecessary, inconsistent comment in
<xlocmon>.