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 OverflowSafeInt are via the OverflowSafeInt32 and OverflowSafeInt64 typedefs. Would there be any objection to simplifying the template to use std::numeric_limits<T> instead of (the impression of) freely-selectable saturation limits?
My concern is that the current implementation assumes that T_MIN is negative and T_MAX is positive, and that both are of similar magnitude (whereas the previous implementation assumed that T_MIN == -T_MAX); and with user-selected limits, that assumption may be invalid. At the very least, we could enforce that the type parameter is a signed integer type.

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.

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

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

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?

There are two issues that I cannot seem to resolve:

1. Why does the regression test fail with what looks like a bunch of off-by-one errors when I implement a templated constructor that accepts any integral type?
2. Just what exactly is MSVC's problem? Why does it choke so badly on this?

I feel like the changes in this PR have gone a bit "out of scope". Can you explain the RawValue additions? Why is operator() not sufficient?

I feel like the changes in this PR have gone a bit "out of scope". Can you explain the RawValue additions? Why is operator() not sufficient?

Good question. By requiring explicit typecasting from OverflowSafeInt to regular overflow-unsafe integrals, I was able to uncover a number of bugs and potential problems that were introduced by unintentional implicit typecasts to overflow-unsafe integers.

I had originally intended to use explicit operator T () for this purpose, but it soon became evident that the type to be cast is often not readily apparent from the context in which it is used. Perhaps a better method name will suggest itself?

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 OverflowSafeInt, the data type required for the correct result should be obvious.

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 RawValue() method and any other code quality issues.

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 OverflowSafeInt, the data type required for the correct result should be obvious.

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?

Is this PR still relevant? The money underflow exploit described in #8284 is fixed in JGRPP 0.42.0 but not in master as of commit 513641f.