-
-
Notifications
You must be signed in to change notification settings - Fork 951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite OverflowSafeInt
to avoid undefined behaviour
#8285
Conversation
I don't think the regression test suite exercises OverflowSafeInt as it is. I can't think of a good way to do that without major changes to the test, but it's probably worth setting as a goal. |
I'm willing to write tests for it, if somebody can point me in the direction of the documentation for the test suite. Presumably, these make use of the squirrel API in some shape or form? |
Is there any documentation for the tests at all? It works by loading a specific savegame with an AI script, the AI script then does various actions and writes various values to logging. After executing for a number of game ticks the game exits, and the console output is compared to an expected output. So there's three parts to potentially modify: The AI script to add/change behaviours to exercise. The expected output if the expected behaviour changes. The savegame if some basic premises need to be changed. (Changing the savegame is probably rather risky.) |
Nope. I'm not touching that with a 10-foot barge pole 😎. |
Yeah, it would be difficult to get close to over/underflow testing with the AI scripts anyway - certainly extremely time consuming (talking hours) and not something that's feasible for regression tests |
I guess the code is nowhere near modular enough for a TDD design with dependency injection to ever be feasible? |
With e97dd52, the only instances of |
Ideally this check would just be a static_assert, but that is probably asking a bit much when targeting C++11. A command line switch which ran some unit-style tests, output any required log messages, and then exited with the appropriate exit code would be a simple/automatable way to test things like this which don't require a full game state. |
Go for it, don't see why not |
Can't say I'm a fan of a "command line switch" solution to doing unit tests - I don't really want test code in the actual "game", as it were. A separate executable that actually runs/tests the non-side-effecty bits that you actually can run unit tests on should be the preferred solution. |
If said executable could (optionally) be built and run during the compilation process; that would be even better 😄 |
For really simple stuff, static_assert/assert_compile should be preferred, of course |
Could a specific regression test (with different savegame and AI scripts) be prepared just to test for those boundary conditions, where the AI company's money is hacked to be near INT64_MIN? |
cd05c4e
to
e7b43bf
Compare
There are two issues that I cannot seem to resolve:
|
1927bc2
to
c2b660c
Compare
8ffc8bf
to
48f6b22
Compare
I feel like the changes in this PR have gone a bit "out of scope". Can you explain the |
Good question. By requiring explicit typecasting from I had originally intended to use Having said that, a templated explicit typecast to integral might be plausible; as it seems reasonable that, in situations where it makes sense to cast away from |
48f6b22
to
8a7949d
Compare
This fixes, unmasks, and generally avoids a whole class of bugs where an `OverflowSafeInt` is implicitly typecast to a regular int and used in calculations.
Allow copy-construction of OverflowSafeInt from any overflow-safe or non-overflow-safe integral value. Allow comparison to any overflow-safe or non-overflow-safe integral to produce correct results. Allow mixed arithmetic with other overflow-safe or non-overflow-safe integrals with sensible automatic promotion or widening.
8a7949d
to
843dfff
Compare
Now that the program actually compiles under MSVC and the regression tests pass, I'm more than happy to address any concerns you may have with the |
Allow OverflowSafeInt to be explicitly typecast to any integral type. The value is clamped to fit within the range of the target integral type.
Hmm, I'm not convinced this is better. What are your thoughts, @LordAro? |
@LordAro would you be more comfortable if I were to separate the bugfixes for the implicit typecasts into a supplimentary pull request; given that it's something that probably should be fixed but leaving them out won't make anything more broken than it already is? |
The current implementation of
OverflowSafeInt
does not handle the maximally-negative boundary condition very well. This rewrite uses an unsigned type to calculate and represent absolute values, to avoid invoking undefined behaviours such asabs(INT64_MIN) < 0
and-INT64_MIN == INT64_MIN
. Fixes #8284.