Change shift operators to throw on undefined behavior instead of assert #64
Conversation
There was a problem hiding this comment.
Thanks for this, this does fix my original issue.
Just wondering, any invalid bitshifts caught here would be UB, not (possibly defined) overflow, but SafeInt only exposes overflow and div by zero errors. Does it make sense to introduce dedicated handlers for UB via a breaking change or is that too hard to disentagle cleanly anyway?
| @@ -324,7 +324,7 @@ The shift operators have the following signatures: | |||
| SafeInt< U, E > operator >>( U lhs, SafeInt< T, E > bits ) | |||
|
|
|||
| Note: | |||
| Shift operations where the right argument is larger than the number of bits in the left argument type minus 1 is implementation-defined behavior. SafeInt will attempt to help you avoid this by causing an assert in debug builds. If you prefer to have a stronger reaction, create a custom SAFEINT_ASSERT that throws or causes a fault in both debug and release builds. Shifting by a negative bit count also doesn't make sense, and is also trapped. | |||
| Shift operations where the right argument is larger than the number of bits in the left argument type minus 1 is implementation-defined behavior. Undefined behavior, such as shifting by a negative count or more bits than the type has will throw an exception. | |||
There was a problem hiding this comment.
NB: I had missed that SAFEINT_ASSERT was a customization point which might have been a way to get around this.
Even with that info I believe having shift operations throw would still be preferable since that can provide more context (e.g., that we ran into an overflow instead of some other assertion failure).
There was a problem hiding this comment.
So it's technically implementation defined, not UB, which is slightly different. With UB, the compiler can upload your source to the Internet. With implementation defined, it has to do something, compiler vendor decides what.
I think treating it as an overflow is reasonable.
And yes, prior to this you could have made a different ShiftAssert and caused the same as with this fix.
|
@dcleblanc, is this something you would be willing to merge? |
We now use a fork of `SafeInt` that includes dcleblanc/SafeInt#64 (as well as general bump to upstream `master`).
We now use a fork of `SafeInt` that includes dcleblanc/SafeInt#64 (as well as general bump to upstream `master`).
The class had assertions for UB on shift, turn the asserts into exceptions.