mach: Use if constexpr to wrap protected exception handling cases [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
I don't love this. Is there a better way to do this?
Related details
- David Benjamin
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 9 comments![]( "Open in Gerrit")
Was davidben right? Maybe this should have been dtapuska?
I don't love this. Is there a better way to do this?
I don't love this. Is there a better way to do this?
What do you think of this suggestion?
Wrap the bodies of these cases in a compile-time if constexpr check
so they are only compiled and instantiated when Traits = MachExcTraits,
resolving compile-time type-safety issues.When the changes are made, adjust this accordingly.
case MachExcTraits::kMachMessageIDExceptionRaiseIdentityProtected: {Here’s what I suggest:
1. Remove the `default:` label from this `switch` statement. See (5) below for the new way to handle that case.
2. After this `switch` statement, introduce _a new_ `switch` statement, but put it inside a single `if constexpr` (as on your line 490, modified by my comment there).
3. Move the handling of these two message types into that new `switch` statement.
4. This `case` line, and the one for `kMachMessageIDExceptionRaiseIdentityProtected`, will now be “protected” by the `if constexpr`, so you can rewrite `MachExcTraits` to the more natural and correct `Traits`.
5. Instead of ` default:` label in either `switch` statement, this function will change to put the code formerly in the `default:` label to be beyond both `switch` statements, outside of the `if constexpr`.
It’s a small amount of additional hoisting, but I bet that the diff will remain essentially as clean as it is now, with primarily only whitespace indentation changes. The structure of the code will be cleaner and much more logical. The abuse of `MachExcTraits` values for `exc`-subsystem requests will come to an end.
The new structure may even enable further cleanups.
if constexpr (!std::is_same_v<Traits, MachExcTraits>) {Frame this in the affirmative instead: you want to bail out here if `Traits` is `ExcTraits`.
typename MachExcTraits::ExceptionRaiseIdentityProtectedRequest;This can go back to being `Traits` now.
MachExcTraits::MIGCheckRequestExceptionRaiseIdentityProtected(This too.
typename MachExcTraits::ExceptionRaiseIdentityProtectedReply;And this.
if constexpr (!std::is_same_v<Traits, MachExcTraits>) {Same here, and also with the `MachExcTraits`/`Traits` changes that follow.
Related details
- David Benjamin
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Was davidben right? Maybe this should have been dtapuska?
davidben was right. Context: [link](https://chromium-review.googlesource.com/c/7204428/21#message-b5a9b7b32b6a279f2d5759278aa8abc97f4beb75). Sorry for the noise. Dave.
Related details
- David Benjamin
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
Mark MentovaiWas davidben right? Maybe this should have been dtapuska?
davidben was right. Context: [link](https://chromium-review.googlesource.com/c/7204428/21#message-b5a9b7b32b6a279f2d5759278aa8abc97f4beb75). Sorry for the noise. Dave.
Ah yeah, I bumped into this in the course of https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5843750/comment/45cf0fd5_26af8e02/
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |