FS#1114 - expense type passed in CommandCost form Cmds

Attached to Project: OpenTTD
Opened by Noldo (Noldo) - Wednesday, 08 August 2007, 18:40 GMT
Type Patch
Category Core
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch adds a member to class CommandCost for purpose of using that to return expense type from the command functions. Currently this is done via global variable _yearly_expenses_type and SET_EXPENSES_TYPE macro. AddCost function is not changed which leads to this having its expense type unchanged in the operation.

-New ExpenseType EXPENSES_INVALID is added.
-2 new constructors and a get method for the new member are added for CommandCost. The default value for expense type in costructors is EXPENSES_INVALID, which makes debuging easier. No set method is added due to personal preference and a gut feeling that this way is cleaner.
-SubtractMoneyFromAnyPlayer function is edited to fit the change. It also asserts that all costs that have money effect have a valid expense type.

-All Cmd-functions and some functions that are called from those functions and return CommadCost objects are chaged to the new system.
-Some comments are updated to reflect the changes.

So why all the fuzz?
-Getting rid of a global _yearly_expenses_type
-Making a cleaner and more trackable way to connect expense type with the relevant cost.
This task depends upon

Closed by  Remko Bijker (Rubidium)
Wednesday, 09 January 2008, 16:55 GMT
Reason for closing:  Implemented
Additional comments about closing:  In r11793.
Comment by Noldo (Noldo) - Wednesday, 15 August 2007, 17:43 GMT
So it was buggy as hell, a lot of asserts which is a good thing too because thats why there is the EXPENSES_INVALID default. Fixed the ones I run into excluding one that really puzzles me. To find the reason for that I added an assert to the CommandCost::AddCost function to make sure that all CommandCosts that are added something have a valid ExpensesType. I had to make some changes here and there to comply with the assertion. Didn't find the original problem though so there is still atleast one assert there. I used AIs and fast-forward to get backtraces as they will step on it given enough time.
Comment by Remko Bijker (Rubidium) - Wednesday, 15 August 2007, 18:01 GMT
AIs do not test all commands. Especially the onces related to ships are not tested. Another example of totally untested could would be adding/removing drive through stations.
Comment by Noldo (Noldo) - Wednesday, 15 August 2007, 18:39 GMT
Ok, thanks for the pointer I tested those myself.

Comment by Noldo (Noldo) - Wednesday, 22 August 2007, 04:09 GMT
There was a problem with rendering currencies, but I got that sorted. Also the mysterious assert hasn't appeared for a while. I'm starting to be happy with the results. Have to test more though.
Comment by Remko Bijker (Rubidium) - Monday, 10 September 2007, 20:43 GMT
Coding-style wise:
- do not add trailing whitespace
- conform to the coding style (as on the wiki)
- I think the expense type should be before the cost
- do not "just" change CommandCost to Money as those were put there to safeguard the function from overflowing. Either make a subclass of CommandCost that is overflow safe and show that as a different patch or don't change it to Money.
Comment by Patric Stout (TrueBrain) - Saturday, 03 November 2007, 23:29 GMT
Will there be any update? I hope so...
Comment by Noldo (Noldo) - Sunday, 04 November 2007, 07:35 GMT
Ok, I updated the patch to the current trunk, as expected the new Money was the most work. Coding style is propably as bad as earlier. I'll try to find some time to make it better.
Comment by Patric Stout (TrueBrain) - Sunday, 04 November 2007, 15:50 GMT
As Rubidium said, it would be nice if the first param would be the type, the second the cost. I understand this means a lot of search/replace, but it really would be for the best :)
Comment by Noldo (Noldo) - Monday, 31 December 2007, 19:09 GMT
Ok, finally found the time to do something about this.

I switched the order of parameters. Specific comments about the coding style would be welcome now.
Comment by Jean-Francois Claeys (Belugas) - Thursday, 03 January 2008, 20:37 GMT
pretty cool result, even if it is a VERY big patch :)
Comment by Noldo (Noldo) - Tuesday, 08 January 2008, 20:46 GMT
EXPENSES_INVALID -> INVALID_EXPENSES (That was the convention in the wiki)

Normal updating.
Comment by Remko Bijker (Rubidium) - Tuesday, 08 January 2008, 22:39 GMT
See the annotated diff.
   his.diff (13.2 KiB)
Comment by Noldo (Noldo) - Wednesday, 09 January 2008, 16:36 GMT
Changed the things Rubidium pointed out.
Comment by Remko Bijker (Rubidium) - Wednesday, 09 January 2008, 16:55 GMT
Still a lot of trailing whitespaces :(