FS#5029 - Refactoring of Vehicle::AddToShared/RemoveFromShared and OrderList::AddVehicle/RemoveVehicle
Attached to Project:
OpenTTD
Opened by Adam Olsen (Rhamphoryncus) - Monday, 30 January 2012, 00:25 GMT
Last edited by andythenorth (andythenorth) - Monday, 14 August 2017, 20:31 GMT
Opened by Adam Olsen (Rhamphoryncus) - Monday, 30 January 2012, 00:25 GMT
Last edited by andythenorth (andythenorth) - Monday, 14 August 2017, 20:31 GMT
|
DetailsThis is a refactoring of Vehicle::AddToShared/RemoveFromShared and OrderList::AddVehicle/RemoveVehicle. Specifically, the former are removed in favour of the latter. This lets OrderList manage more of the linked list it exists for.
Comments: I wavered on style of single-statement if-statements. In the end I used the form I preferred, which of course might not be preferred for the rest of the codebase. Vehicle didn't like me friend'ing individual methods in OrderList, I think due to a header conflict (I was probably doing something wrong), So I friend'd the whole OrderList class instead. Vehicle::FirstShared irks me. It should always be the first vehicle in the chain, so the only reason it seems to use this->First() is to avoid const-casting this for the return value. Testing has been limited. It passes "make test", but I haven't put any play time in. 
order.diff
(11.1 KiB)
|
This task depends upon
Closed by andythenorth (andythenorth)
Monday, 14 August 2017, 20:31 GMT
Reason for closing: Won't implement
Additional comments about closing: Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).
Monday, 14 August 2017, 20:31 GMT
Reason for closing: Won't implement
Additional comments about closing: Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).
Comment by Michael Lutz (michi_cc) -
Monday, 30 January 2012, 01:34 GMT
OpenTTD coding style is at https://wiki.openttd.org/Coding_style. You should avoid changing existing code, especially if it is properly formatted like in OrderList::RemoveVehicle().
Comment by Adam Olsen (Rhamphoryncus) -
Monday, 30 January 2012, 06:20 GMT
That came across a little harsh. I made the function consistent with itself, but since only a *single* line wasn't rewritten by me that means matching the style I felt looks best.
Comment by Michael Lutz (michi_cc) -
Monday, 30 January 2012, 11:55 GMT
Okay, yes. Still doesn't change the fact that anything going into trunk has to confirm to trunk coding style.
Comment by Adam Olsen (Rhamphoryncus) -
Monday, 30 January 2012, 14:50 GMT
I knew that already. I mentioned it because I expected it'd be something to fix.
|
|
