search

dcleblanc / SafeInt

SafeInt is a class library for C++ that manages integer overflows.
MIT License
202 stars 37 forks source link

Incorrect implementation of CastThrow for CastToBool #37

Closed ZacharyHenkel closed 3 years ago

ZacharyHenkel commented 3 years ago

Caught by the new clang warning unused-but-set-parameter. Should the body be t = b?

error: parameter 'b' set but not used [-Werror,-Wunused-but-set-parameter] static constexpr void CastThrow( bool b, T& t ) SAFEINT_CPP_THROW

dcleblanc commented 3 years ago

You have found a very interesting bug, and it has been there a very long time. What happened is obvious - I was copying and pasting, and forgot to swap the arguments, as I did in the non-throwing method above.

It should have been:

_CONSTEXPR14 static void CastThrow( T t, bool& b ) SAFEINT_CPP_THROW

So then my next question is how did this not break things, and why are tests not failing? Then I commented it out, to see what could be calling it - turns out the compile test and the runtime tests both compile correctly with this commented out. There apparently are no tests that call this, which is very annoying.

For right now, you should change the function declaration to the above, and I'll go take a look at the test coverage and try to discover how this was missed. I want to check in the fix and the test coverage at the same time.

Thank you very much for bringing this to my attention.

dcleblanc commented 3 years ago

OK, so it isn't actually all that interesting, I was alarmed for nothing, but it was sloppy, here's the fix:

// CastThrow is not needed.
// This is because CastThrow is only ever called inside various multiply, divide, add, and subtract methods, none of which make sense for one of
// the operands to be bool. It is also called within the various direct casts that will cast the SafeInt instance to various types, but there is nothing
// to check for a cast to bool, and it is implemented directly without needing this method.

// There is a call to Cast because it is used in the SafeCast non-throwing function.
dcleblanc commented 3 years ago

Since this was a very minor change, effectively a no-op, I just went ahead and pushed it into main without incrementing the version. It's been pushed, just sync, and the problem should go away.