From 0841978304df05e72539558c5c674db5396a5466 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Sun, 21 Jan 2024 09:21:22 +0000 Subject: [PATCH] Codechange: Use vector and iterators to store old/new vehicles during autoreplace. (#11851) This avoids malloc/free of 3 arrays along index counting, and the data for each part is kept together. --- src/autoreplace_cmd.cpp | 91 +++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 44 deletions(-) diff --git a/src/autoreplace_cmd.cpp b/src/autoreplace_cmd.cpp index c67f3b4092..5d6b6eeaaf 100644 --- a/src/autoreplace_cmd.cpp +++ b/src/autoreplace_cmd.cpp @@ -490,6 +490,21 @@ static CommandCost ReplaceFreeUnit(Vehicle **single_unit, DoCommandFlag flags, b return cost; } +/** Struct for recording vehicle chain replacement information. */ +struct ReplaceChainItem { + Vehicle *old_veh; ///< Old vehicle to replace. + Vehicle *new_veh; ///< Replacement vehicle, or nullptr if no replacement. + Money cost; /// Cost of buying and refitting replacement. + + ReplaceChainItem(Vehicle *old_veh, Vehicle *new_veh, Money cost) : old_veh(old_veh), new_veh(new_veh), cost(cost) { } + + /** + * Get vehicle to use for this position. + * @return Either the new vehicle, or the old vehicle if there is no replacement. + */ + Vehicle *GetVehicle() const { return new_veh == nullptr ? old_veh : new_veh; } +}; + /** * Replace a whole vehicle chain * @param chain vehicle chain to let autoreplace/renew operator on @@ -509,29 +524,21 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon /* Store the length of the old vehicle chain, rounded up to whole tiles */ uint16_t old_total_length = CeilDiv(Train::From(old_head)->gcache.cached_total_length, TILE_SIZE) * TILE_SIZE; - int num_units = 0; ///< Number of units in the chain - for (Train *w = Train::From(old_head); w != nullptr; w = w->GetNextUnit()) num_units++; - - Train **old_vehs = CallocT(num_units); ///< Will store vehicles of the old chain in their order - Train **new_vehs = CallocT(num_units); ///< New vehicles corresponding to old_vehs or nullptr if no replacement - Money *new_costs = MallocT(num_units); ///< Costs for buying and refitting the new vehicles + std::vector replacements; /* Collect vehicles and build replacements * Note: The replacement vehicles can only successfully build as long as the old vehicles are still in their chain */ - int i; - Train *w; - for (w = Train::From(old_head), i = 0; w != nullptr; w = w->GetNextUnit(), i++) { - assert(i < num_units); - old_vehs[i] = w; + for (Train *w = Train::From(old_head); w != nullptr; w = w->GetNextUnit()) { + ReplaceChainItem &replacement = replacements.emplace_back(w, nullptr, 0); - CommandCost ret = BuildReplacementVehicle(old_vehs[i], (Vehicle**)&new_vehs[i], true); + CommandCost ret = BuildReplacementVehicle(replacement.old_veh, &replacement.new_veh, true); cost.AddCost(ret); if (cost.Failed()) break; - new_costs[i] = ret.GetCost(); - if (new_vehs[i] != nullptr) *nothing_to_do = false; + replacement.cost = ret.GetCost(); + if (replacement.new_veh != nullptr) *nothing_to_do = false; } - Train *new_head = (new_vehs[0] != nullptr ? new_vehs[0] : old_vehs[0]); + Vehicle *new_head = replacements.front().GetVehicle(); /* Note: When autoreplace has already failed here, old_vehs[] is not completely initialized. But it is also not needed. */ if (cost.Succeeded()) { @@ -545,18 +552,18 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon * We do this from back to front, so that the head of the temporary vehicle chain does not change all the time. * That way we also have less trouble when exceeding the unitnumber limit. * OTOH the vehicle attach callback is more expensive this way :s */ - Train *last_engine = nullptr; ///< Shall store the last engine unit after this step + Vehicle *last_engine = nullptr; ///< Shall store the last engine unit after this step if (cost.Succeeded()) { - for (int i = num_units - 1; i > 0; i--) { - Train *append = (new_vehs[i] != nullptr ? new_vehs[i] : old_vehs[i]); + for (auto it = std::rbegin(replacements); it != std::rend(replacements); ++it) { + Vehicle *append = it->GetVehicle(); if (RailVehInfo(append->engine_type)->railveh_type == RAILVEH_WAGON) continue; - if (new_vehs[i] != nullptr) { + if (it->new_veh != nullptr) { /* Move the old engine to a separate row with DC_AUTOREPLACE. Else * moving the wagon in front may fail later due to unitnumber limit. * (We have to attach wagons without DC_AUTOREPLACE.) */ - CmdMoveVehicle(old_vehs[i], nullptr, DC_EXEC | DC_AUTOREPLACE, false); + CmdMoveVehicle(it->old_veh, nullptr, DC_EXEC | DC_AUTOREPLACE, false); } if (last_engine == nullptr) last_engine = append; @@ -567,15 +574,15 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon } /* When wagon removal is enabled and the new engines without any wagons are already longer than the old, we have to fail */ - if (cost.Succeeded() && wagon_removal && new_head->gcache.cached_total_length > old_total_length) cost = CommandCost(STR_ERROR_TRAIN_TOO_LONG_AFTER_REPLACEMENT); + if (cost.Succeeded() && wagon_removal && Train::From(new_head)->gcache.cached_total_length > old_total_length) cost = CommandCost(STR_ERROR_TRAIN_TOO_LONG_AFTER_REPLACEMENT); /* Append/insert wagons into the new vehicle chain * We do this from back to front, so we can stop when wagon removal or maximum train length (i.e. from mammoth-train setting) is triggered. */ if (cost.Succeeded()) { - for (int i = num_units - 1; i > 0; i--) { + for (auto it = std::rbegin(replacements); it != std::rend(replacements); ++it) { assert(last_engine != nullptr); - Vehicle *append = (new_vehs[i] != nullptr ? new_vehs[i] : old_vehs[i]); + Vehicle *append = it->GetVehicle(); if (RailVehInfo(append->engine_type)->railveh_type == RAILVEH_WAGON) { /* Insert wagon after 'last_engine' */ @@ -584,7 +591,7 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon /* When we allow removal of wagons, either the move failing due * to the train becoming too long, or the train becoming longer * would move the vehicle to the empty vehicle chain. */ - if (wagon_removal && (res.Failed() ? res.GetErrorMessage() == STR_ERROR_TRAIN_TOO_LONG : new_head->gcache.cached_total_length > old_total_length)) { + if (wagon_removal && (res.Failed() ? res.GetErrorMessage() == STR_ERROR_TRAIN_TOO_LONG : Train::From(new_head)->gcache.cached_total_length > old_total_length)) { CmdMoveVehicle(append, nullptr, DC_EXEC | DC_AUTOREPLACE, false); break; } @@ -594,16 +601,16 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon } else { /* We have reached 'last_engine', continue with the next engine towards the front */ assert(append == last_engine); - last_engine = last_engine->GetPrevUnit(); + last_engine = Train::From(last_engine)->GetPrevUnit(); } } } /* Sell superfluous new vehicles that could not be inserted. */ if (cost.Succeeded() && wagon_removal) { - assert(new_head->gcache.cached_total_length <= _settings_game.vehicle.max_train_length * TILE_SIZE); - for (int i = 1; i < num_units; i++) { - Vehicle *wagon = new_vehs[i]; + assert(Train::From(new_head)->gcache.cached_total_length <= _settings_game.vehicle.max_train_length * TILE_SIZE); + for (auto it = std::next(std::begin(replacements)); it != std::end(replacements); ++it) { + Vehicle *wagon = it->new_veh; if (wagon == nullptr) continue; if (wagon->First() == new_head) break; @@ -612,11 +619,11 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon /* Sell wagon */ [[maybe_unused]] CommandCost ret = Command::Do(DC_EXEC, wagon->index, false, false, INVALID_CLIENT_ID); assert(ret.Succeeded()); - new_vehs[i] = nullptr; + it->new_veh = nullptr; /* Revert the money subtraction when the vehicle was built. * This value is different from the sell value, esp. because of refitting */ - cost.AddCost(-new_costs[i]); + cost.AddCost(-it->cost); } } @@ -631,8 +638,8 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon } /* Transfer cargo of old vehicles and sell them */ - for (int i = 0; i < num_units; i++) { - Vehicle *w = old_vehs[i]; + for (auto it = std::begin(replacements); it != std::end(replacements); ++it) { + Vehicle *w = it->old_veh; /* Is the vehicle again part of the new chain? * Note: We cannot test 'new_vehs[i] != nullptr' as wagon removal might cause to remove both */ if (w->First() == new_head) continue; @@ -644,8 +651,8 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon * it from failing due to engine limits. */ cost.AddCost(Command::Do(flags | DC_AUTOREPLACE, w->index, false, false, INVALID_CLIENT_ID)); if ((flags & DC_EXEC) != 0) { - old_vehs[i] = nullptr; - if (i == 0) old_head = nullptr; + it->old_veh = nullptr; + if (it == std::begin(replacements)) old_head = nullptr; } } @@ -662,8 +669,8 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon assert(Train::From(old_head)->GetNextUnit() == nullptr); - for (int i = num_units - 1; i > 0; i--) { - [[maybe_unused]] CommandCost ret = CmdMoveVehicle(old_vehs[i], old_head, DC_EXEC | DC_AUTOREPLACE, false); + for (auto it = std::rbegin(replacements); it != std::rend(replacements); ++it) { + [[maybe_unused]] CommandCost ret = CmdMoveVehicle(it->old_veh, old_head, DC_EXEC | DC_AUTOREPLACE, false); assert(ret.Succeeded()); } } @@ -671,17 +678,13 @@ static CommandCost ReplaceChain(Vehicle **chain, DoCommandFlag flags, bool wagon /* Finally undo buying of new vehicles */ if ((flags & DC_EXEC) == 0) { - for (int i = num_units - 1; i >= 0; i--) { - if (new_vehs[i] != nullptr) { - Command::Do(DC_EXEC, new_vehs[i]->index, false, false, INVALID_CLIENT_ID); - new_vehs[i] = nullptr; + for (auto it = std::rbegin(replacements); it != std::rend(replacements); ++it) { + if (it->new_veh != nullptr) { + Command::Do(DC_EXEC, it->new_veh->index, false, false, INVALID_CLIENT_ID); + it->new_veh = nullptr; } } } - - free(old_vehs); - free(new_vehs); - free(new_costs); } else { /* Build and refit replacement vehicle */ Vehicle *new_head = nullptr;