Added a check for overflowing player credits

[MorMund]

Fixes #7348.

[penev92]

That looks like a huge overkill to me, with regards to performance.

[MorMund]

That looks like a huge overkill to me, with regards to performance.

I don't think there will be a large (if any) performance impact. For reference :
Bear in mind that this has nothing to do with try/catch blocks: you only incur the cost when the actual exception is thrown. You can use as many try/catch blocks as you want.

And checking for overflows in the first place is also virtually free perfomance wise.

[phrohdoh]

I think we should fix the problem instead of just throwing when it occurs.

[phrohdoh]

Logging*, not throwing.

[MorMund]

The problem is "fixed" by this. If the integer overflows the value isn't saved i. e. if you gain credits beyond the limit the "transaction" is cancelled.

[penev92]

Just in case we decide to go with this:

1. You should probably do each addition in a separate try
2. In the catch set the variable to the max value of the type. Like you max your silos. And there is really no point in logging here.

[obrakmann]

I agree with penev here: setting the affected variable to int.MaxValue and then not logging would probably be the best thing to do. This is only for the benefit of people who go trigger-happy on the debug cheats, anyway (which is why a similar check for Resources and Earned in GiveResources() probably isn't necessary).

[RoosterDragon]

It may be worth considering the following instead of a try-catch too.

if ((long)X + num > int.MaxValue)
    X = int.MaxValue;
else
    X += num;

[ScottNZ]

As people have noted above:
I don't think this log is necessary, but either way cought is spelled incorrectly.
It should also set the respective value to int.MaxValue if there is an overflow.
I also think the checked block would be better inside the try block.

[phrohdoh]

Or you could add ints and check < 0

[MorMund]

Applied most of the suggestions made here.

[obrakmann]

Works as advertised, 👍

[penev92]

In what scenario is this false ?
This if is redundant. It would probably hurt performance more that it would help.

[pchote]

This isn't redundant - it prevents the (otherwise guaranteed) exception throw and catch if the limit has already been reached. The perf overhead of the if() is completely negligible.

[MorMund]

This if is redundant. It would probably hurt performance more that it would help.

Without the if a OverflowException would be thrown everytime you gain money after you've hit the limit.

[Unit158]

The variable Cash can't be over int.MaxValue already, so that check does nothing.

On 2015-02-12, at 1:52 PM, DeadlySurprise notifications@github.com wrote:

This if is redundant. It would probably hurt performance more that it would help.

Without the if a OverflowException would be thrown everytime you gain money after you've hit the limit.

�
Reply to this email directly or view it on GitHub.

[abcdefg30]

But it can be int.MaxValue.

In this case you can skip the whole check.

[MunWolf]

The cases where it is int.MaxValue are not common enough to justify a check for it so better to just have the try blocks on cash and earned.
For example I doubt this check will ever be false except in a dev enviorment.

[LipkeGu]

@wolfgaming this needs to be proper fixed! as @phrohdoh and @ScottNZ already said in irc =/ instead of hiding Code from the Debugger!

[penev92]

It does need to be fixed. But I cannot give a +1 with the current ifs.
If checked is as fast as I was told, remove the surrounding ifs and I'd be OK with a try/catch containing a checked.

[MorMund]

I’m very surprised how concerned you all are about 2 if-statements. The performance cost of simply calling GiveCash() (not executing just calling!) is more than double that of both ifs. The argument that the cash limit is only hit in extreme cases is of course true, but the performance hit is so miniscule that there is no reason imho to have real performance issues once you hit the limit.

[Unit158]

No, we are concerned about the unneeded clutter they provide. Nor Earned nor Cash may be greater than int.MaxValue. It isn't mathematically possible. Even if they are equal, the ratio of the performance gain to the amount of clutter added is not significant.

[penev92]

The argument that the cash limit is only hit in extreme cases is of course true, but the performance hit is so miniscule that there is no reason imho to have real performance issues once you hit the limit.

My concern is the performance difference (however small) before reaching that limit.
Anyway, I said that I cannot give a +1 like this. Anyone else can feel free to do it.
Also I don't see what you have against removing the ifs.

[LipkeGu]

We should not use try & catch :(

[pchote]

I'm confused by the bike shedding here - this looks fine to me. I don't have time to test this, otherwise I would merge this myself.

[MunWolf]

@LipkeGu Only way I can see this being done without a try catch is changing it to long and then casting it to int again, and I like that worse then the try-catch.

@DeadlySurprise The main problem with the ifs is that they add like 6 lines of code that are unneccesary since it is already handled by the try-catch.

Also as a question has this exception ever been reached except when using a cheat command?

[pchote]

Finally made time to test this, and everything checks out 👍 Thanks.

[pchote]

https://github.com/OpenRA/OpenRA/wiki/Changelog/_compare/03eef14fac6bc68083f6d08efa2d0b316b9c82b4%5E...03eef14fac6bc68083f6d08efa2d0b316b9c82b4