(svn r2290) - CodeChange: protect the next batch of commands. This brings us to a total of 61, which is 53% :)

- CodeChange: To correctly accept engine-prototypes, the best-player checking has been moved to its own function, I hope it functions the same as before.
- CodeChange: Added symbolic types of PlayerID, OrderID and EngineID. For engines also added GetEngine() and IsEngineIndex(), similar to the other such functions.
- CodeChange: To correctly build industries, some tables have been moved to build_industry.h. The only way to find out currently if an industry is valid in a climate is by looping all industries and checking if it matches. Also to comply with the patch setting build_rawmaterial_industries, it is assumed that these industries do not accept any cargo of any type. This can and probably should changed in the future to some flag in their struct. Also use _opt_ptr instead of _opt.
- CodeChange: implemented the HQ checking code inspired by MarkR2 in "[ 1190944 ] Many commands not checked for security".  Unfortunately it is impossible to prevent only deleting a HQ by a modified client atm.
- CodeChange: For insert order and modify order their parameters are implicitely truncated to 8 bits, instead of the 16 bits said in the comments.

aircraft_cmd.c

@@ -358,7 +358,11 @@ static void DoDeleteAircraft(Vehicle *v)
 	InvalidateWindowClasses(WC_AIRCRAFT_LIST);
 }
 
-// p1 = vehicle
+/** Sell an aircraft.
+ * @param x,y unused
+ * @param p1 vehicle ID to be sold
+ * @param p2 unused
+ */
 int32 CmdSellAircraft(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
@@ -383,7 +387,11 @@ int32 CmdSellAircraft(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return -(int32)v->value;
 }
 
-// p1 = vehicle
+/** Start/Stop an aircraft.
+ * @param x,y unused
+ * @param p1 aircraft to start/stop
+ * @param p2 unused
+ */
 int32 CmdStartStopAircraft(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
@@ -392,13 +400,11 @@ int32 CmdStartStopAircraft(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 
 	v = GetVehicle(p1);
 
-	if (v->type != VEH_Aircraft || !CheckOwnership(v->owner))
+	if (v->type != VEH_Aircraft || !CheckOwnership(v->owner)) return CMD_ERROR;
-		return CMD_ERROR;
 
 	// cannot stop airplane when in flight, or when taking off / landing
-	if (v->u.air.state >= STARTTAKEOFF) {
+	if (v->u.air.state >= STARTTAKEOFF)
 		return_cmd_error(STR_A017_AIRCRAFT_IS_IN_FLIGHT);
-	}
 
 	if (flags & DC_EXEC) {
 		v->vehstatus ^= VS_STOPPED;

command.c

@@ -208,7 +208,6 @@ static CommandProc * const _command_proc_table[] = {
 	CmdTrainGotoDepot,						/* 39 */
 	CmdForceTrainProceed,					/* 40 */
 	CmdReverseTrainDirection,			/* 41 */
-	/***************************************************/
 
 	CmdModifyOrder,								/* 42 */
 	CmdSkipOrder,									/* 43 */
@@ -237,6 +236,7 @@ static CommandProc * const _command_proc_table[] = {
 
 	CmdSellAircraft,							/* 59 */
 	CmdStartStopAircraft,					/* 60 */
+	/***************************************************/
 	CmdBuildAircraft,							/* 61 */
 	CmdSendAircraftToHangar,			/* 62 */
 	CmdChangeAircraftServiceInt,	/* 63 */
@@ -302,7 +302,7 @@ static CommandProc * const _command_proc_table[] = {
 	CmdStartScenario,							/* 109 */
 	CmdBuildSignalTrack,					/* 110 */
 	CmdRemoveSignalTrack,					/* 111 */
-	CmdDestroyCompanyHQ,					/* 112 */
+	CmdDestroyCompanyHQ,					/* 112    <-- done */
 	CmdGiveMoney,									/* 113 */
 	CmdChangePatchSetting,				/* 114 */
 	CmdReplaceVehicle,						/* 115 */

engine.c

@@ -646,34 +646,53 @@ StringID GetCustomEngineName(int engine)
 }
 
 
-void AcceptEnginePreview(Engine *e, int player)
+void AcceptEnginePreview(Engine *e, PlayerID player)
 {
-	Player *p;
+	Player *p = DEREF_PLAYER(player);
 
 	SETBIT(e->player_avail, player);
 
-	p = DEREF_PLAYER(player);
+	UPDATE_PLAYER_RAILTYPE(e, p);
-
-	UPDATE_PLAYER_RAILTYPE(e,p);
 
 	e->preview_player = 0xFF;
 	InvalidateWindowClasses(WC_BUILD_VEHICLE);
 	InvalidateWindowClasses(WC_REPLACE_VEHICLE);
 }
 
+static PlayerID GetBestPlayer(PlayerID pp)
+{
+	const Player *p;
+	int32 best_hist;
+	PlayerID best_player;
+	uint mask = 0;
+
+	do {
+		best_hist = -1;
+		best_player = -1;
+		FOR_ALL_PLAYERS(p) {
+			if (p->is_active && p->block_preview == 0 && !HASBIT(mask, p->index) &&
+					p->old_economy[0].performance_history > best_hist) {
+				best_hist = p->old_economy[0].performance_history;
+				best_player = p->index;
+			}
+		}
+
+		if (best_player == (PlayerID)-1) return -1;
+
+		SETBIT(mask, best_player);
+	} while (--pp != 0);
+
+	return best_player;
+}
+
 void EnginesDailyLoop(void)
 {
 	Engine *e;
-	int i,num;
+	int i;
-	Player *p;
-	uint mask;
-	int32 best_hist;
-	int best_player;
 
-	if (_cur_year >= 130)
+	if (_cur_year >= 130) return;
-		return;
 
-	for(e=_engines,i=0; i!=TOTAL_NUM_ENGINES; e++,i++) {
+	for (e = _engines, i = 0; i != TOTAL_NUM_ENGINES; e++, i++) {
 		if (e->flags & ENGINE_INTRODUCING) {
 			if (e->flags & ENGINE_PREVIEWING) {
 				if (e->preview_player != 0xFF && !--e->preview_wait) {
@@ -681,47 +700,45 @@ void EnginesDailyLoop(void)
 					DeleteWindowById(WC_ENGINE_PREVIEW, i);
 					e->preview_player++;
 				}
-			} else if (e->preview_player != 0xFF) {
+ 			} else if (e->preview_player != 0xFF) {
-				num = e->preview_player;
+				PlayerID best_player = GetBestPlayer(e->preview_player);
-				mask = 0;
+
-				do {
+				if (best_player == (PlayerID)-1) {
-					best_hist = -1;
+					e->preview_player = 0xFF;
-					best_player = -1;
+					continue;
-					FOR_ALL_PLAYERS(p) {
+				}
-						if (p->is_active && p->block_preview == 0 && !HASBIT(mask,p->index) &&
-								p->old_economy[0].performance_history > best_hist) {
-							best_hist = p->old_economy[0].performance_history;
-							best_player = p->index;
-						}
-					}
-					if (best_player == -1) {
-						e->preview_player = 0xFF;
-						goto next_engine;
-					}
-					mask |= (1 << best_player);
-				} while (--num != 0);
 
 				if (!IS_HUMAN_PLAYER(best_player)) {
-					/* TTDBUG: TTD has a bug here */
+					/* XXX - TTDBUG: TTD has a bug here ???? */
 					AcceptEnginePreview(e, best_player);
 				} else {
 					e->flags |= ENGINE_PREVIEWING;
 					e->preview_wait = 20;
-					if (IS_INTERACTIVE_PLAYER(best_player)) {
+					if (IS_INTERACTIVE_PLAYER(best_player))
 						ShowEnginePreviewWindow(i);
-					}
 				}
 			}
 		}
-		next_engine:;
 	}
 }
 
+/** Accept an engine prototype. XXX - it is possible that the top-player
+ * changes while you are waiting to accept the offer? Then it becomes invalid
+ * @param x,y unused
+ * @param p1 engine-prototype offered
+ * @param p2 unused
+ */
 int32 CmdWantEnginePreview(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	if (flags & DC_EXEC) {
+	Engine *e;
-		AcceptEnginePreview(&_engines[p1], _current_player);
+	if (!IsEngineIndex(p1)) return CMD_ERROR;
-	}
+
+	e = DEREF_ENGINE(p1);
+	if (GetBestPlayer(e->preview_player) != _current_player) return CMD_ERROR;
+
+	if (flags & DC_EXEC)
+		AcceptEnginePreview(e, _current_player);
+
 	return 0;
 }
 
@@ -818,13 +835,19 @@ void EnginesMonthlyLoop(void)
 	AdjustAvailAircraft();
 }
 
+/** Rename an engine.
+ * @param x,y unused
+ * @param p1 engine ID to rename
+ * @param p2 unused
+ */
 int32 CmdRenameEngine(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	StringID str;
 
+	if (!IsEngineIndex(p1)) return CMD_ERROR;
+
 	str = AllocateNameUnique((const char*)_decode_parameters, 0);
-	if (str == 0)
+	if (str == 0) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
 		StringID old_str = _engine_name_strings[p1];
@@ -926,7 +949,7 @@ bool IsEngineBuildable(uint engine, byte type)
 	const Engine *e;
 
 	// check if it's an engine that is in the engine array
-	if (engine >= TOTAL_NUM_ENGINES) return false;
+	if (!IsEngineIndex(engine)) return false;
 
 	e = DEREF_ENGINE(engine);
 

engine.h

@@ -66,10 +66,10 @@ typedef struct Engine {
 	uint16 duration_phase_1, duration_phase_2, duration_phase_3;
 	byte lifelength;
 	byte flags;
-	byte preview_player;
+	PlayerID preview_player;
 	byte preview_wait;
 	byte railtype;
-	byte player_avail;
+	PlayerID player_avail;
 	byte type;				// type, ie VEH_Road, VEH_Train, etc. Same as in vehicle.h
 } Engine;
 
@@ -128,7 +128,7 @@ void DrawRoadVehEngineInfo(int engine, int x, int y, int maxw);
 void DrawShipEngineInfo(int engine, int x, int y, int maxw);
 void DrawAircraftEngineInfo(int engine, int x, int y, int maxw);
 
-void AcceptEnginePreview(Engine *e, int player);
+void AcceptEnginePreview(Engine *e, PlayerID player);
 
 void LoadCustomEngineNames(void);
 void DeleteCustomEngineNames(void);
@@ -150,9 +150,21 @@ enum {
 	ROAD_ENGINES_INDEX = NUM_TRAIN_ENGINES,
 };
 VARDEF Engine _engines[TOTAL_NUM_ENGINES];
-#define DEREF_ENGINE(i) (&_engines[i])
+#define FOR_ALL_ENGINES(e) for (e = _engines; e != endof(_engines); e++)
+#define DEREF_ENGINE(i) (GetEngine(i))
+static inline Engine* GetEngine(uint i)
+{
+  assert(i < lengthof(_engines));
+  return &_engines[i];
+}
+
 VARDEF StringID _engine_name_strings[TOTAL_NUM_ENGINES];
 
+static inline bool IsEngineIndex(uint index)
+{
+	return index < TOTAL_NUM_ENGINES;
+}
+
 /* Access Vehicle Data */
 //#include "table/engines.h"
 extern EngineInfo _engine_info[TOTAL_NUM_ENGINES];

industry_cmd.c

@@ -64,18 +64,13 @@ typedef struct DrawIndustrySpec4Struct {
 	byte image_3;
 } DrawIndustrySpec4Struct;
 
-
-
-#include "table/industry_land.h"
-
-
 typedef struct IndustryTileTable {
 	TileIndexDiffC ti;
 	byte map5;
 } IndustryTileTable;
 
 typedef struct IndustrySpec {
-	const IndustryTileTable * const * table;
+	const IndustryTileTable *const *table;
 	byte num_table;
 	byte a,b,c;
 	byte produced_cargo[2];
@@ -84,6 +79,7 @@ typedef struct IndustrySpec {
 	byte check_proc;
 } IndustrySpec;
 
+#include "table/industry_land.h"
 #include "table/build_industry.h"
 
 typedef enum IndustryType {
@@ -1549,47 +1545,59 @@ static void DoCreateNewIndustry(Industry *i, uint tile, int type, const Industry
 	InvalidateWindow(WC_INDUSTRY_DIRECTORY, 0);
 }
 
-/* p1 = industry type 0-36 */
+/** Build/Fund an industry
+ * @param x,y coordinates where industry is built
+ * @param p1 industry type @see build_industry.h and @see industry.h
+ * @param p2 unused
+ */
 int32 CmdBuildIndustry(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	uint tile = TILE_FROM_XY(x,y);
 	Town *t;
+	Industry *i;
+	TileIndex tile = TILE_FROM_XY(x,y);
 	int num;
 	const IndustryTileTable * const *itt;
 	const IndustryTileTable *it;
-	Industry *i;
 	const IndustrySpec *spec;
 
 	SET_EXPENSES_TYPE(EXPENSES_OTHER);
 
-	if (!CheckSuitableIndustryPos(tile))
+	if (!CheckSuitableIndustryPos(tile)) return CMD_ERROR;
-		return CMD_ERROR;
+
+	/* Check if the to-be built/founded industry is available for this climate.
+	 * Unfortunately we have no easy way of checking, except for looping the table */
+	{ const byte *i;
+		bool found = false;
+		for (i = &_build_industry_types[_opt_ptr->landscape][0]; i != endof(_build_industry_types[_opt_ptr->landscape]); i++) {
+			if (*i == p1) {found = true; break;}
+		}
+		if (!found) return CMD_ERROR;
+	}
 
 	spec = &_industry_spec[p1];
+	/* If the patch for non-raw-material industries is not on, you cannot build raw-material industries.
+	 * Raw material industries are industries that do not accept cargo (at least for now) */
+	if (!_patches.build_rawmaterial_ind && spec->accepts_cargo[0] == 255 &&
+		  spec->accepts_cargo[1] == 255 && spec->accepts_cargo[2] == 255) return CMD_ERROR;
 
-	if (!_check_new_industry_procs[spec->check_proc](tile, p1))
+	if (!_check_new_industry_procs[spec->check_proc](tile, p1)) return CMD_ERROR;
-		return CMD_ERROR;
 
-	if ((t=CheckMultipleIndustryInTown(tile, p1)) == NULL)
+	if ((t = CheckMultipleIndustryInTown(tile, p1)) == NULL) return CMD_ERROR;
-		return CMD_ERROR;
 
 	num = spec->num_table;
 	itt = spec->table;
 
 	do {
-		if (--num < 0)
+		if (--num < 0) return_cmd_error(STR_0239_SITE_UNSUITABLE);
-			return_cmd_error(STR_0239_SITE_UNSUITABLE);
+	} while (!CheckIfIndustryTilesAreFree(tile, it = itt[num], p1, t));
-	} while (!CheckIfIndustryTilesAreFree(tile, it=itt[num], p1, t));
 
 
-	if (!CheckIfTooCloseToIndustry(tile, p1))
+	if (!CheckIfTooCloseToIndustry(tile, p1)) return CMD_ERROR;
-		return CMD_ERROR;
 
-	if ( (i = AllocateIndustry()) == NULL)
+	if ( (i = AllocateIndustry()) == NULL) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC)
-		DoCreateNewIndustry(i, tile, p1, it, t, 0x10);
+		DoCreateNewIndustry(i, tile, p1, it, t, OWNER_NONE);
 
 	return (_price.build_industry >> 5) * _industry_type_costs[p1];
 }

industry_gui.c

@@ -12,13 +12,8 @@
 #include "industry.h"
 #include "town.h"
 
-static const byte _build_industry_types[4][12] = {
+/* Present in table/build_industry.h" */
-	{ 1,  2, 4,  6, 8, 0, 3, 5,  9, 11, 18 },
+extern const byte _build_industry_types[4][12];
-	{ 1, 14, 4, 13, 7, 0, 3, 9, 11, 15 },
-	{ 25, 13, 4, 23, 22, 11, 17, 10, 24, 19, 20, 21 },
-	{ 27, 30, 31, 33, 26, 28, 29, 32, 34, 35, 36 },
-};
-
 extern const byte _industry_type_costs[37];
 
 static void UpdateIndustryProduction(Industry *i);
@@ -30,7 +25,7 @@ static void BuildIndustryWndProc(Window *w, WindowEvent *e)
 	case WE_PAINT:
 		DrawWindowWidgets(w);
 		if (_thd.place_mode == 1 && _thd.window_class == WC_BUILD_INDUSTRY) {
-			int ind_type = _build_industry_types[_opt.landscape][WP(w,def_d).data_1];
+			int ind_type = _build_industry_types[_opt_ptr->landscape][WP(w,def_d).data_1];
 
 			SetDParam(0, (_price.build_industry >> 5) * _industry_type_costs[ind_type]);
 			DrawStringCentered(85, w->height - 21, STR_482F_COST, 0);
@@ -46,7 +41,7 @@ static void BuildIndustryWndProc(Window *w, WindowEvent *e)
 	} break;
 
 	case WE_PLACE_OBJ:
-		if (DoCommandP(e->place.tile, _build_industry_types[_opt.landscape][WP(w,def_d).data_1], 0, NULL, CMD_BUILD_INDUSTRY | CMD_MSG(STR_4830_CAN_T_CONSTRUCT_THIS_INDUSTRY)))
+		if (DoCommandP(e->place.tile, _build_industry_types[_opt_ptr->landscape][WP(w,def_d).data_1], 0, NULL, CMD_BUILD_INDUSTRY | CMD_MSG(STR_4830_CAN_T_CONSTRUCT_THIS_INDUSTRY)))
 			ResetObjectToPlace();
 		break;
 
@@ -267,7 +262,7 @@ static const WindowDesc * const _industry_window_desc[2][4] = {
 
 void ShowBuildIndustryWindow(void)
 {
-	AllocateWindowDescFront(_industry_window_desc[_patches.build_rawmaterial_ind][_opt.landscape],0);
+	AllocateWindowDescFront(_industry_window_desc[_patches.build_rawmaterial_ind][_opt_ptr->landscape],0);
 }
 
 #define NEED_ALTERB	((_game_mode == GM_EDITOR || _cheats.setup_prod.value) && (i->accepts_cargo[0] == CT_INVALID || i->accepts_cargo[0] == CT_VALUABLES))

misc_cmd.c

@@ -11,10 +11,11 @@
 #include "economy.h"
 #include "network.h"
 
-/* p1 = player
+/** Change the player's face.
-   p2 = face
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 face bitmasked
  */
-
 int32 CmdSetPlayerFace(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	if (flags & DC_EXEC) {
@@ -24,16 +25,18 @@ int32 CmdSetPlayerFace(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return 0;
 }
 
-/* p1 = player
+/** Change the player's company-colour
- * p2 = color
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 new colour for vehicles, property, etc.
  */
 int32 CmdSetPlayerColor(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	Player *p,*pp;
+	Player *p, *pp;
 
 	p = GetPlayer(_current_player);
 
-	/* ensure no dups */
+	/* Ensure no two companies have the same colour */
 	FOR_ALL_PLAYERS(pp) {
 		if (pp->is_active && pp != p && pp->player_color == (byte)p2)
 			return CMD_ERROR;
@@ -47,10 +50,14 @@ int32 CmdSetPlayerColor(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return 0;
 }
 
+/** Increase the loan of your company.
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 when set, loans the maximum amount in one go (press CTRL)
+ */
 int32 CmdIncreaseLoan(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Player *p;
-	int32 size;
 
 	p = GetPlayer(_current_player);
 
@@ -60,13 +67,11 @@ int32 CmdIncreaseLoan(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	}
 
 	if (flags & DC_EXEC) {
-		if (p2)
+		/* Loan the maximum amount or not? */
-			size = _economy.max_loan - p->current_loan;
+		int32 loan = (p2) ? _economy.max_loan - p->current_loan : IS_HUMAN_PLAYER(_current_player) ? 10000 : 50000;
-		else
-			size = IS_HUMAN_PLAYER(_current_player) ? 10000 : 50000;
 
-		p->money64 += size;
+		p->money64 += loan;
-		p->current_loan += size;
+		p->current_loan += loan;
 		UpdatePlayerMoney32(p);
 		InvalidatePlayerWindows(p);
 	}
@@ -74,55 +79,61 @@ int32 CmdIncreaseLoan(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return 0;
 }
 
+/** Decrease the loan of your company.
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 when set, pays back the maximum loan permitting money (press CTRL)
+ */
 int32 CmdDecreaseLoan(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Player *p;
-	int32 size;
+	int32 loan;
 
 	p = GetPlayer(_current_player);
 
-	if (p->current_loan == 0)
+	if (p->current_loan == 0) return_cmd_error(STR_702D_LOAN_ALREADY_REPAYED);
-		return_cmd_error(STR_702D_LOAN_ALREADY_REPAYED);
 
-	size = p->current_loan;
+	loan = p->current_loan;
 
-	// p2 is true while CTRL is pressed (repay all possible loan, or max money you have)
+	/* p2 is true while CTRL is pressed (repay all possible loan, or max money you have)
-	if (!p2) {
+	 * Repay any loan in chunks of 10.000 pounds */
-	    if (_patches.ainew_active)
+	if (p2) {
- 		    size = min(size, 10000);
+		loan = min(loan, p->player_money);
-	    else
+		loan = max(loan, 10000);
-		    size = min(size, IS_HUMAN_PLAYER(_current_player) ? 10000 : 50000);
+		loan -= loan % 10000;
-	} else {	// only repay in chunks of 10K
+	} else {
-		size = min(size, p->player_money);
+		loan = (_patches.ainew_active) ? min(loan, 10000) : min(loan, IS_HUMAN_PLAYER(_current_player) ? 10000 : 50000);
-		size = max(size, 10000);
-		size -= size % 10000;
 	}
 
-	if (p->player_money < size) {
+	if (p->player_money < loan) {
-		SetDParam(0, size);
+		SetDParam(0, loan);
 		return_cmd_error(STR_702E_REQUIRED);
 	}
 
 	if (flags & DC_EXEC) {
-		p->money64 -= size;
+		p->money64 -= loan;
-		p->current_loan -= size;
+		p->current_loan -= loan;
 		UpdatePlayerMoney32(p);
 		InvalidatePlayerWindows(p);
 	}
 	return 0;
 }
 
+/** Change the name of the company.
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 unused
+ */
 int32 CmdChangeCompanyName(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	StringID str,old_str;
 	Player *p;
 
 	str = AllocateNameUnique((const char*)_decode_parameters, 4);
-	if (str == 0)
+	if (str == 0) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
-		p = DEREF_PLAYER(p1);
+		p = DEREF_PLAYER(_current_player);
 		old_str = p->name_1;
 		p->name_1 = str;
 		DeleteName(old_str);
@@ -134,24 +145,27 @@ int32 CmdChangeCompanyName(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return 0;
 }
 
+/** Change the name of the president.
+ * @param x,y unused
+ * @param p1 unused
+ * @param p2 unused
+ */
 int32 CmdChangePresidentName(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	StringID str,old_str;
 	Player *p;
 
 	str = AllocateNameUnique((const char*)_decode_parameters, 4);
-	if (str == 0)
+	if (str == 0) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
-		p = DEREF_PLAYER(p1);
+		p = DEREF_PLAYER(_current_player);
 		old_str = p->president_name_1;
 		p->president_name_1 = str;
 		DeleteName(old_str);
 
 		if (p->name_1 == STR_SV_UNNAMED) {
-			ttd_strlcat(
+			ttd_strlcat((char*)_decode_parameters, " Transport", sizeof(_decode_parameters));
-				(char*)_decode_parameters, " Transport", sizeof(_decode_parameters));
 			DoCommandByTile(0, p1, 0, DC_EXEC, CMD_CHANGE_COMPANY_NAME);
 		}
 		MarkWholeScreenDirty();

order.h

@@ -69,7 +69,7 @@ typedef struct Order {
 
 typedef struct {
 	VehicleID clone;
-	byte orderindex;
+	OrderID orderindex;
 	Order order[41];
 	uint16 service_interval;
 	char name[32];

order_cmd.c

@@ -140,28 +140,31 @@ void AssignOrder(Order *order, Order data)
 	order->station = data.station;
 }
 
-/**
+/** Add an order to the orderlist of a vehicle.
- *
+ * @param x,y unused
- * Add an order to the orderlist of a vehicle
+ * @param p1 various bitstuffed elements
- *
+ * - p1 = (bit  0 - 15) - ID of the vehicle (p1 & 0xFFFF)
- * @param veh_sel      First 16 bits are the ID of the vehicle. The next 16 are the selected order (if any)
+ * - p1 = (bit 16 - 31) - the selected order (if any). If the last order is given,
- *                       If the lastone is given, order will be inserted above thatone
+ *                        the order will be inserted before that one (p1 & 0xFFFF0000)>>16
- * @param packed_order Packed order to insert
+ *                        only the first 8 bytes used currently (bit 16 - 23) (max 255)
- *
+ * @param p2 packed order to insert
  */
-int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_order)
+int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
-	int sel         = veh_sel >> 16;
+	VehicleID veh = p1 & 0xFFFF;
-	Order new_order = UnpackOrder(packed_order);
+	OrderID sel_ord = p1 >> 16;
+	Order new_order = UnpackOrder(p2);
 
-	if (!IsVehicleIndex(veh_sel & 0xFFFF)) return CMD_ERROR;
+	if (!IsVehicleIndex(veh)) return CMD_ERROR;
-	v = GetVehicle(veh_sel & 0xFFFF);
+	v = GetVehicle(veh);
 	if (v->type == 0 || !CheckOwnership(v->owner)) return CMD_ERROR;
 
+	/* Check if the inserted order is to the correct destination (owner, type),
+	 * and has the correct flags if any */
 	switch (new_order.type) {
 		case OT_GOTO_STATION: {
-			const Station* st;
+			const Station *st;
 
 			if (!IsStationIndex(new_order.station)) return CMD_ERROR;
 			st = GetStation(new_order.station);
@@ -192,8 +195,7 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 					if (!(st->facilities & FACIL_AIRPORT)) return CMD_ERROR;
 					break;
 
-				default:
+				default: return CMD_ERROR;
-					return CMD_ERROR;
 			}
 
 			switch (new_order.flags) {
@@ -205,8 +207,7 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 				case OF_NON_STOP | OF_UNLOAD:
 					break;
 
-				default:
+				default: return CMD_ERROR;
-					return CMD_ERROR;
 			}
 			break;
 		}
@@ -248,8 +249,7 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 						if (!IsTileDepotType(dp->xy, TRANSPORT_WATER)) return CMD_ERROR;
 						break;
 
-					default:
+					default: return CMD_ERROR;
-						return CMD_ERROR;
 				}
 			}
 
@@ -260,8 +260,7 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 				case OF_NON_STOP | OF_PART_OF_ORDERS | OF_HALT_IN_DEPOT:
 					break;
 
-				default:
+				default: return CMD_ERROR;
-					return CMD_ERROR;
 			}
 			break;
 		}
@@ -281,35 +280,30 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 				case OF_NON_STOP:
 					break;
 
-				default:
+				default: return CMD_ERROR;
-					return CMD_ERROR;
 			}
 			break;
 		}
 
-		default:
+		default: return CMD_ERROR;
-			return CMD_ERROR;
 	}
 
-	if (sel > v->num_orders)
+	if (sel_ord > v->num_orders) return CMD_ERROR;
-		return_cmd_error(STR_EMPTY);
 
-	if (IsOrderPoolFull())
+	if (IsOrderPoolFull()) return_cmd_error(STR_8831_NO_MORE_SPACE_FOR_ORDERS);
-		return_cmd_error(STR_8831_NO_MORE_SPACE_FOR_ORDERS);
 
 	/* XXX - This limit is only here because the backuppedorders can't
-	    handle any more then this.. */
+	 * handle any more then this.. */
-	if (v->num_orders >= 40)
+	if (v->num_orders >= 40) return_cmd_error(STR_8832_TOO_MANY_ORDERS);
-		return_cmd_error(STR_8832_TOO_MANY_ORDERS);
 
 	/* For ships, make sure that the station is not too far away from the
 	 * previous destination, for human players with new pathfinding disabled */
 	if (v->type == VEH_Ship && IS_HUMAN_PLAYER(v->owner) &&
-		sel != 0 && GetVehicleOrder(v, sel - 1)->type == OT_GOTO_STATION
+		sel_ord != 0 && GetVehicleOrder(v, sel_ord - 1)->type == OT_GOTO_STATION
 		&& !_patches.new_pathfinding_all) {
 
 		int dist = DistanceManhattan(
-			GetStation(GetVehicleOrder(v, sel - 1)->station)->xy,
+			GetStation(GetVehicleOrder(v, sel_ord - 1)->station)->xy,
 			GetStation(new_order.station)->xy // XXX type != OT_GOTO_STATION?
 		);
 		if (dist >= 130)
@@ -317,10 +311,8 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 	}
 
 	if (flags & DC_EXEC) {
-		Order *new;
 		Vehicle *u;
-
+		Order *new = AllocateOrder();
-		new = AllocateOrder();
 		AssignOrder(new, new_order);
 
 		/* Create new order and link in list */
@@ -329,9 +321,9 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 		} else {
 			/* Try to get the previous item (we are inserting above the
 			    selected) */
-			Order *order = GetVehicleOrder(v, sel - 1);
+			Order *order = GetVehicleOrder(v, sel_ord - 1);
 
-			if (order == NULL && GetVehicleOrder(v, sel) != NULL) {
+			if (order == NULL && GetVehicleOrder(v, sel_ord) != NULL) {
 				/* There is no previous item, so we are altering v->orders itself
 				    But because the orders can be shared, we copy the info over
 				    the v->orders, so we don't have to change the pointers of
@@ -356,14 +348,13 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 			u->num_orders++;
 
 			/* If the orderlist was empty, assign it */
-			if (u->orders == NULL)
+			if (u->orders == NULL) u->orders = v->orders;
-				u->orders = v->orders;
 
 			assert(v->orders == u->orders);
 
 			/* If there is added an order before the current one, we need
 			to update the selected order */
-			if (sel <= u->cur_order_index) {
+			if (sel_ord <= u->cur_order_index) {
 				uint cur = u->cur_order_index + 1;
 				/* Check if we don't go out of bound */
 				if (cur < u->num_orders)
@@ -382,10 +373,9 @@ int32 CmdInsertOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 packed_o
 	return 0;
 }
 
-/**
+/** Declone an order-list
- *
+ * @param *dst delete the orders of this vehicle
- * Declone an order-list
+ * @param flags execution flags
- *
  */
 static int32 DecloneOrder(Vehicle *dst, uint32 flags)
 {
@@ -399,50 +389,43 @@ static int32 DecloneOrder(Vehicle *dst, uint32 flags)
 	return 0;
 }
 
-/**
+/** Delete an order from the orderlist of a vehicle.
- *
+ * @param x,y unused
- * Delete an order from the orderlist of a vehicle
+ * @param p1 the ID of the vehicle
- *
+ * @param p2 the order to delete (max 255)
- * @param vehicle_id The ID of the vehicle
- * @param selected   The order to delete
- *
  */
-int32 CmdDeleteOrder(int x, int y, uint32 flags, uint32 vehicle_id, uint32 selected)
+int32 CmdDeleteOrder(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	Vehicle *v;
+	Vehicle *v, *u;
-	Vehicle *u;
+	VehicleID veh_id = p1;
-	uint sel   = selected;
+	OrderID sel_ord = p2;
 	Order *order;
 
-	if (!IsVehicleIndex(vehicle_id)) return CMD_ERROR;
+	if (!IsVehicleIndex(veh_id)) return CMD_ERROR;
-	v = GetVehicle(vehicle_id);
+	v = GetVehicle(veh_id);
 	if (v->type == 0 || !CheckOwnership(v->owner)) return CMD_ERROR;
 
-	/* XXX -- Why is this here? :s */
-	_error_message = STR_EMPTY;
-
 	/* If we did not select an order, we maybe want to de-clone the orders */
-	if (sel >= v->num_orders)
+	if (sel_ord >= v->num_orders)
 		return DecloneOrder(v, flags);
 
-	order = GetVehicleOrder(v, sel);
+	order = GetVehicleOrder(v, sel_ord);
-	if (order == NULL)
+	if (order == NULL) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
-		if (GetVehicleOrder(v, sel - 1) == NULL) {
+		if (GetVehicleOrder(v, sel_ord - 1) == NULL) {
-			if (GetVehicleOrder(v, sel + 1) != NULL) {
+			if (GetVehicleOrder(v, sel_ord + 1) != NULL) {
 				/* First item, but not the last, so we need to alter v->orders
 				    Because we can have shared order, we copy the data
 				    from the next item over the deleted */
-				order = GetVehicleOrder(v, sel + 1);
+				order = GetVehicleOrder(v, sel_ord + 1);
 				SwapOrders(v->orders, order);
 			} else {
 				/* Last item, so clean the list */
 				v->orders = NULL;
 			}
 		} else {
-			GetVehicleOrder(v, sel - 1)->next = order->next;
+			GetVehicleOrder(v, sel_ord - 1)->next = order->next;
 		}
 
 		/* Give the item free */
@@ -453,20 +436,18 @@ int32 CmdDeleteOrder(int x, int y, uint32 flags, uint32 vehicle_id, uint32 selec
 		while (u != NULL) {
 			u->num_orders--;
 
-			if (sel < u->cur_order_index)
+			if (sel_ord < u->cur_order_index)
 				u->cur_order_index--;
 
 			/* If we removed the last order, make sure the shared vehicles
-			    also set their orders to NULL */
+			 * also set their orders to NULL */
-			if (v->orders == NULL)
+			if (v->orders == NULL) u->orders = NULL;
-				u->orders = NULL;
 
 			assert(v->orders == u->orders);
 
 			/* NON-stop flag is misused to see if a train is in a station that is
-			    on his order list or not */
+			 * on his order list or not */
-			if (sel == u->cur_order_index &&
+			if (sel_ord == u->cur_order_index && u->current_order.type == OT_LOADING &&
-					u->current_order.type == OT_LOADING &&
 					HASBIT(u->current_order.flags, OFB_NON_STOP)) {
 				u->current_order.flags = 0;
 			}
@@ -483,43 +464,35 @@ int32 CmdDeleteOrder(int x, int y, uint32 flags, uint32 vehicle_id, uint32 selec
 	return 0;
 }
 
-/**
+/** Goto next order of order-list.
- *
+ * @param x,y unused
- * Goto next order of order-list
+ * @param p1 The ID of the vehicle which order is skipped
- *
+ * @param p2 unused
- * @param vehicle_id The ID of the vehicle
- *
  */
-int32 CmdSkipOrder(int x, int y, uint32 flags, uint32 vehicle_id, uint32 not_used)
+int32 CmdSkipOrder(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
+	VehicleID veh_id = p1;
 
-	if (!IsVehicleIndex(vehicle_id)) return CMD_ERROR;
+	if (!IsVehicleIndex(veh_id)) return CMD_ERROR;
-	v = GetVehicle(vehicle_id);
+	v = GetVehicle(veh_id);
 	if (v->type == 0 || !CheckOwnership(v->owner)) return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
 		/* Goto next order */
-		{
+		OrderID b = v->cur_order_index + 1;
-			byte b = v->cur_order_index + 1;
+		if (b >= v->num_orders) b = 0;
-			if (b >= v->num_orders)
-				b = 0;
 
-			v->cur_order_index = b;
+		v->cur_order_index = b;
 
-			if (v->type == VEH_Train)
+		if (v->type == VEH_Train) v->u.rail.days_since_order_progr = 0;
-				v->u.rail.days_since_order_progr = 0;
 
-			if (v->type == VEH_Road)
+		if (v->type == VEH_Road) ClearSlot(v, v->u.road.slot);
-				ClearSlot(v, v->u.road.slot);
-		}
 
 		/* NON-stop flag is misused to see if a train is in a station that is
-		    on his order list or not */
+		 * on his order list or not */
-		if (v->current_order.type == OT_LOADING &&
+		if (v->current_order.type == OT_LOADING && HASBIT(v->current_order.flags, OFB_NON_STOP))
-				HASBIT(v->current_order.flags, OFB_NON_STOP)) {
 			v->current_order.flags = 0;
-		}
 
 		InvalidateVehicleOrder(v);
 	}
@@ -532,37 +505,39 @@ int32 CmdSkipOrder(int x, int y, uint32 flags, uint32 vehicle_id, uint32 not_use
 }
 
 
-/**
+/** Modify an order in the orderlist of a vehicle.
- *
+ * @param x,y unused
- * Modify an order in the orderlist of a vehicle
+ * @param p1 various bitstuffed elements
- *
+ * - p1 = (bit  0 - 15) - ID of the vehicle (p1 & 0xFFFF)
- * @param veh_sel      First 16 bits are the ID of the vehicle. The next 16 are the selected order (if any)
+ * - p1 = (bit 16 - 31) - the selected order (if any). If the last order is given,
- *                       If the lastone is given, order will be inserted above thatone
+ *                        the order will be inserted before that one (p1 & 0xFFFF0000)>>16
- * @param mode         Mode to change the order to
+ *                        only the first 8 bytes used currently (bit 16 - 23) (max 255)
- *
+ * @param p2 mode to change the order to (always set)
  */
-int32 CmdModifyOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 mode)
+int32 CmdModifyOrder(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
-	byte sel = veh_sel >> 16;
 	Order *order;
+	OrderID sel_ord = p1 >> 16; // XXX - automatically truncated to 8 bits.
+	VehicleID veh = p1 & 0xFFFF;
 
-	if (!IsVehicleIndex(veh_sel & 0xFFFF)) return CMD_ERROR;
+	if (!IsVehicleIndex(veh)) return CMD_ERROR;
-	v = GetVehicle(veh_sel & 0xFFFF);
+	if (p2 != OFB_FULL_LOAD || p2 != OFB_UNLOAD || p2 != OFB_NON_STOP) return CMD_ERROR;
+
+	v = GetVehicle(veh);
 	if (v->type == 0 || !CheckOwnership(v->owner)) return CMD_ERROR;
 
 	/* Is it a valid order? */
-	if (sel >= v->num_orders)
+	if (sel_ord >= v->num_orders) return CMD_ERROR;
-		return CMD_ERROR;
 
-	order = GetVehicleOrder(v, sel);
+	order = GetVehicleOrder(v, sel_ord);
 	if (order->type != OT_GOTO_STATION &&
-			(order->type != OT_GOTO_DEPOT || mode == OFB_UNLOAD) &&
+		 (order->type != OT_GOTO_DEPOT || p2 == OFB_UNLOAD) &&
-			(order->type != OT_GOTO_WAYPOINT || mode != OFB_NON_STOP))
+		 (order->type != OT_GOTO_WAYPOINT || p2 != OFB_NON_STOP))
 		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
-		switch (mode) {
+		switch (p2) {
 		case OFB_FULL_LOAD:
 			TOGGLEBIT(order->flags, OFB_FULL_LOAD);
 			if (order->type != OT_GOTO_DEPOT)
@@ -575,9 +550,7 @@ int32 CmdModifyOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 mode)
 		case OFB_NON_STOP:
 			TOGGLEBIT(order->flags, OFB_NON_STOP);
 			break;
-
+		default: NOT_REACHED();
-			default:
-				return CMD_ERROR;
 		}
 
 		/* Update the windows and full load flags, also for vehicles that share the same order list */
@@ -585,7 +558,7 @@ int32 CmdModifyOrder(int x, int y, uint32 flags, uint32 veh_sel, uint32 mode)
 			Vehicle *u = GetFirstVehicleFromSharedList(v);
 			while (u != NULL) {
 				/* toggle u->current_order "Full load" flag if it changed */
-				if (sel == u->cur_order_index &&
+				if (sel_ord == u->cur_order_index &&
 						HASBIT(u->current_order.flags, OFB_FULL_LOAD) != HASBIT(order->flags, OFB_FULL_LOAD))
 					TOGGLEBIT(u->current_order.flags, OFB_FULL_LOAD);
 				InvalidateVehicleOrder(u);

player.h

@@ -159,7 +159,7 @@ typedef struct Player {
 	byte player_money_fraction;
 	byte max_railtype;
 	byte block_preview;
-	byte index;
+	PlayerID index;
 
 	uint16 cargo_types; /* which cargo types were transported the last year */
 

player_gui.c

@@ -626,9 +626,9 @@ static void PlayerCompanyWndProc(Window *w, WindowEvent *e)
 		break;
 
 	case WE_PLACE_OBJ: {
-		// you cannot destroy a HQ, only relocate it. So build_HQ is called, just with different flags
+		/* You cannot destroy a HQ, only relocate it. So build_HQ is called, just with different flags */
 		TileIndex tile = DEREF_PLAYER(w->window_number)->location_of_house;
-		if (DoCommandP(e->place.tile, (tile == 0) ? 0 : (1 << 16) | w->window_number, 0, NULL, CMD_BUILD_COMPANY_HQ | CMD_AUTO | CMD_NO_WATER | CMD_MSG(STR_7071_CAN_T_BUILD_COMPANY_HEADQUARTERS)))
+		if (DoCommandP(e->place.tile, (tile == 0) ? 0 : 1 | w->window_number, 0, NULL, CMD_BUILD_COMPANY_HQ | CMD_AUTO | CMD_NO_WATER | CMD_MSG(STR_7071_CAN_T_BUILD_COMPANY_HEADQUARTERS)))
 			ResetObjectToPlace();
 		break;
 	}

station_cmd.c

@@ -1698,7 +1698,7 @@ int32 CmdBuildAirport(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	h = _airport_size_y[p1];
 
 	cost = CheckFlatLandBelow(tile, w, h, flags, 0, NULL);
-	if (cost == CMD_ERROR) return CMD_ERROR;
+	if (CmdFailed(cost)) return CMD_ERROR;
 
 	st = GetStationAround(tile, w, h, -1);
 	if (st == CHECK_STATIONS_ERR) return CMD_ERROR;
@@ -2643,6 +2643,11 @@ static void UpdateStationWaiting(Station *st, int type, uint amount)
 	InvalidateWindow(WC_STATION_VIEW, st->index);
 }
 
+/** Rename a station
+ * @param x,y unused
+ * @param p1 station ID that is to be renamed
+ * @param p2 unused
+ */
 int32 CmdRenameStation(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	StringID str,old_str;
@@ -2654,8 +2659,7 @@ int32 CmdRenameStation(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	if (!IsValidStation(st) || !CheckOwnership(st->owner)) return CMD_ERROR;
 
 	str = AllocateNameUnique((const char*)_decode_parameters, 6);
-	if (str == 0)
+	if (str == 0) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
 		old_str = st->string_id;

table/build_industry.h

@@ -1111,6 +1111,13 @@ const	byte _industry_type_costs[37] = {
 	187, 193,  17,  20, 192,  22, 203, 213, 210
 };
 
+const byte _build_industry_types[4][12] = {
+	{ 1,  2, 4,  6, 8, 0, 3, 5,  9, 11, 18 },
+	{ 1, 14, 4, 13, 7, 0, 3, 9, 11, 15 },
+	{ 25, 13, 4, 23, 22, 11, 17, 10, 24, 19, 20, 21 },
+	{ 27, 30, 31, 33, 26, 28, 29, 32, 34, 35, 36 },
+};
+
 
 static const byte _industry_create_table_0[] = {
 	4, 4,

train_cmd.c

@@ -1478,8 +1478,10 @@ int32 CmdTrainGotoDepot(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return 0;
 }
 
-/* p1 = vehicle
+/** Change the service interval for trains.
- * p2 = new service int
+ * @param x,y unused
+ * @param p1 vehicle ID that is being service-interval-changed
+ * @param p2 new service interval (0 - 65536)
  */
 int32 CmdChangeTrainServiceInt(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
@@ -1489,8 +1491,7 @@ int32 CmdChangeTrainServiceInt(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 
 	v = GetVehicle(p1);
 
-	if (v->type != VEH_Train || !CheckOwnership(v->owner))
+	if (v->type != VEH_Train || !CheckOwnership(v->owner)) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
 		v->service_interval = (uint16)p2;

ttd.h

@@ -65,6 +65,8 @@ typedef struct NewsItem NewsItem;
 typedef struct Industry Industry;
 typedef struct DrawPixelInfo DrawPixelInfo;
 typedef uint16 VehicleID;
+typedef byte PlayerID;
+typedef byte OrderID;
 typedef uint16 StringID;
 typedef uint16 SpriteID;
 typedef uint32 PalSpriteID;

unmovable_cmd.c

@@ -111,7 +111,7 @@ static int32 ClearTile_Unmovable(uint tile, byte flags)
 
 	if (m5 & 0x80) {
 		if (_current_player == OWNER_WATER)
-			return DoCommandByTile(tile, OWNER_WATER, 0, DC_EXEC, CMD_DESTROY_COMPANY_HQ);
+			return DoCommandByTile(tile, 0, 0, DC_EXEC, CMD_DESTROY_COMPANY_HQ);
 		return_cmd_error(STR_5804_COMPANY_HEADQUARTERS_IN);
 	}
 
@@ -309,58 +309,45 @@ restart:
 
 extern int32 CheckFlatLandBelow(uint tile, uint w, uint h, uint flags, uint invalid_dirs, int *);
 
-/* p1      = relocate HQ
+/** Build or relocate the HQ. This depends if the HQ is already built or not
- * p1&0xFF = player whose HQ is up for relocation
+ * @param x,y the coordinates where the HQ will be built or relocated to
+ * @param p1 relocate HQ (set to some value, usually 1 or true)
+ * @param p2 unused
  */
 int32 CmdBuildCompanyHQ(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	TileIndex tile = TILE_FROM_XY(x,y);
 	Player *p = DEREF_PLAYER(_current_player);
-	int score;
+	int cost;
-	int32 cost = 0;
 
 	SET_EXPENSES_TYPE(EXPENSES_PROPERTY);
 
 	cost = CheckFlatLandBelow(tile, 2, 2, flags, 0, NULL);
+	if (CmdFailed(cost)) return CMD_ERROR;
 
-	if (cost == CMD_ERROR)
+	if (p1) { /* Moving HQ */
-		return CMD_ERROR;
+		int32 ret;
 
-	if (p1) {
+		if (p->location_of_house == 0) return CMD_ERROR;
-		int32 ret = DoCommand(
-			TileX(p->location_of_house) * 16, TileY(p->location_of_house) * 16,
-			p1 & 0xFF, 0, flags, CMD_DESTROY_COMPANY_HQ);
 
-		if (ret == CMD_ERROR)
+		ret = DoCommandByTile(p->location_of_house, 0, 0, flags, CMD_DESTROY_COMPANY_HQ);
-			return CMD_ERROR;
+
+		if (CmdFailed(ret)) return CMD_ERROR;
 
 		cost += ret;
+	} else { /* Building new HQ */
+		if (p->location_of_house != 0) return CMD_ERROR;
 	}
 
 	if (flags & DC_EXEC) {
-		score = UpdateCompanyRatingAndValue(p, false);
+		int score = UpdateCompanyRatingAndValue(p, false);
 
 		p->location_of_house = tile;
 
-		ModifyTile(tile + TILE_XY(0,0),
+		ModifyTile(tile + TILE_XY(0,0), MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5, 0x80);
-			MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5,
+		ModifyTile(tile + TILE_XY(0,1), MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5, 0x81);
-			0x80
+		ModifyTile(tile + TILE_XY(1,0), MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5, 0x82);
-		);
+		ModifyTile(tile + TILE_XY(1,1), MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5, 0x83);
-
-		ModifyTile(tile + TILE_XY(0,1),
-			MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5,
-			0x81
-		);
-
-		ModifyTile(tile + TILE_XY(1,0),
-			MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5,
-			0x82
-		);
-
-		ModifyTile(tile + TILE_XY(1,1),
-			MP_SETTYPE(MP_UNMOVABLE) | MP_MAPOWNER_CURRENT | MP_MAP5,
-			0x83
-		);
 		UpdatePlayerHouse(p, score);
 		InvalidateWindow(WC_COMPANY, (int)p->index);
 	}
@@ -368,17 +355,24 @@ int32 CmdBuildCompanyHQ(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	return cost;
 }
 
-/*	p1 = owner of the HQ */
+/** Destroy a HQ.
+ * During normal gameplay you can only implicitely destroy a HQ when you are
+ * rebuilding it. Otherwise, only water can destroy it. Unfortunately there is
+ * no safeguard against a hacked client to call this command, unless we also add
+ * flags to the command table which commands can be called directly and which not.
+ * @param x,y tile coordinates where HQ is located to destroy
+ * @param p1 unused
+ * @param p2 unused
+ */
 int32 CmdDestroyCompanyHQ(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	uint tile = TILE_FROM_XY(x,y);
+	TileIndex tile = TILE_FROM_XY(x,y);
 	Player *p;
 
 	SET_EXPENSES_TYPE(EXPENSES_PROPERTY);
 
-	if ((int)p1 != OWNER_WATER)	// destruction was initiated by player
+	/* Find player that has HQ flooded, and reset their location_of_house */
-		p = DEREF_PLAYER((byte)p1);
+	if (_current_player == OWNER_WATER)	{
-	else {	// find player that has HQ flooded, and reset their location_of_house
 		bool dodelete = false;
 		FOR_ALL_PLAYERS(p) {
 			if (p->location_of_house == tile) {
@@ -386,9 +380,9 @@ int32 CmdDestroyCompanyHQ(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 				break;
 			}
 		}
-		if (!dodelete)
+		if (!dodelete) return CMD_ERROR;
-			return CMD_ERROR;
+	} else  /* Destruction was initiated by player */
-	}
+		p = DEREF_PLAYER(_current_player);
 
 	if (flags & DC_EXEC) {
 		p->location_of_house = 0; // reset HQ position

vehicle.c

@@ -1621,7 +1621,11 @@ void MaybeReplaceVehicle(Vehicle *v)
 	_current_player = OWNER_NONE;
 }
 
-
+/** Give a custom name to your vehicle
+ * @param x,y unused
+ * @param p1 vehicle ID to name
+ * @param p2 unused
+ */
 int32 CmdNameVehicle(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
@@ -1631,12 +1635,10 @@ int32 CmdNameVehicle(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 
 	v = GetVehicle(p1);
 
-	if (!CheckOwnership(v->owner))
+	if (!CheckOwnership(v->owner)) return CMD_ERROR;
-		return CMD_ERROR;
 
 	str = AllocateNameUnique((const char*)_decode_parameters, 2);
-	if (str == 0)
+	if (str == 0) return CMD_ERROR;
-		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
 		StringID old_str = v->string_id;

vehicle.h

@@ -142,7 +142,7 @@ struct Vehicle {
 	StringID string_id; // Displayed string
 
 	UnitID unitnumber;	// unit number, for display purposes only
-	byte owner;				// which player owns the vehicle?
+	PlayerID owner;				// which player owns the vehicle?
 
 	TileIndex tile;		// Current tile index
 	TileIndex dest_tile; // Heading for this tile
@@ -188,14 +188,14 @@ struct Vehicle {
 	byte tick_counter;// increased by one for each tick
 
 	/* Begin Order-stuff */
-	Order current_order;  //! The current order (+ status, like: loading)
+	Order current_order;     //! The current order (+ status, like: loading)
-	byte cur_order_index; //! The index to the current order
+	OrderID cur_order_index; //! The index to the current order
 
-	Order *orders;        //! Pointer to the first order for this vehicle
+	Order *orders;           //! Pointer to the first order for this vehicle
-	byte num_orders;      //! How many orders there are in the list
+	OrderID num_orders;      //! How many orders there are in the list
 
-	Vehicle *next_shared; //! If not NULL, this points to the next vehicle that shared the order
+	Vehicle *next_shared;    //! If not NULL, this points to the next vehicle that shared the order
-	Vehicle *prev_shared; //! If not NULL, this points to the prev vehicle that shared the order
+	Vehicle *prev_shared;    //! If not NULL, this points to the prev vehicle that shared the order
 	/* End Order-stuff */
 
 	// Boundaries for the current position in the world and a next hash link.