GH-41994 [C++]: kernel.cc: Remove defaults on switch so that compiler can check full enum coverage for us #41995
Conversation
…enum coverage for us
|
|
cpp/src/arrow/compute/kernel.cc
Outdated
| } | ||
| return true; |
There was a problem hiding this comment.
This means that if kind_ is poisoned memory, we return true instead of poison (true or false non-deterministically). Making the code identical to the previous version but without the default case.
github-actions
bot
added
awaiting changes
| } | ||
| return false; |
There was a problem hiding this comment.
Can we add a comment about "must not be reached" or something here?
There was a problem hiding this comment.
It might be reached if kind_ is bogus memory. I could remove this line altogether and let poison (true/false randomly) be returned.
There was a problem hiding this comment.
I'm assuming @wesm was intentional in having the default case here to protect against that kind of UB.
I checked this on godbolt: https://godbolt.org/z/qEaGMGdzh
cpp/src/arrow/compute/kernel.cc
Outdated
| // ANY_TYPE | ||
| return true; | ||
| case InputType::ANY_TYPE: | ||
| break; |
There was a problem hiding this comment.
It's not a strong opinion but I like using return true here for consistency of other cases in this switch:
| break; | |
| return true; |
There was a problem hiding this comment.
OK. I'm changing this to true and false after the switch.
| break; | ||
| } | ||
| return resolver_(ctx, types); |
There was a problem hiding this comment.
How about returning an error by default?
| break; | |
| } | |
| return resolver_(ctx, types); | |
| return resolver_(ctx, types); | |
| } | |
| return Status::NotImplemented("Must not happen"); // UnknownError? |
There was a problem hiding this comment.
Overkill. That return Status would only ever be reached if kind_ is poisoned memory: in that case all that we need is consistent behavior to not go crazy debugging.
| case OutputType::COMPUTED: | ||
| break; | ||
| } | ||
| return "computed"; |
There was a problem hiding this comment.
| case OutputType::COMPUTED: | |
| break; | |
| } | |
| return "computed"; | |
| case OutputType::COMPUTED: | |
| return "computed"; | |
| } | |
| return "unknown"; |
There was a problem hiding this comment.
Same point as above.
github-actions
bot
added
awaiting merge
github-actions
bot
added
awaiting change review
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 164be48. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
To let the compiler warn us about missing cases and make the non-handled cases more obvious.
What changes are included in this PR?
Removal of
defaultin the switches and improving some dchecks with a message.Are these changes tested?
By existing tests.