Review: Approve
Coede LGTM - just some small nits. Needs testing.
Diff comments:
>
> === modified file 'src/ai/defaultai_warfare.cc'
> --- src/ai/defaultai_warfare.cc 2017-01-06 09:00:11 +0000
> +++ src/ai/defaultai_warfare.cc 2017-01-22 16:41:24 +0000
> @@ -478,22 +478,26 @@
> // reducing ware queues
> // - for armours and weapons to 1
> // - for others to 6
> - std::vector<WaresQueue*> const warequeues1 = tso.site->warequeues();
> - size_t nr_warequeues = warequeues1.size();
> - for (size_t i = 0; i < nr_warequeues; ++i) {
> + // - for others to 6
> + std::vector<InputQueue*> const inputqueues1 = tso.site->inputqueues();
> + for (InputQueue *queue : inputqueues1) {
How about:
for (InputQueue *queue : tso.site->inputqueues()) {
and lose the extra variable?
> +
> + if (queue->get_type() != wwWARE) {
> + continue;
> + }
>
> // if it was decreased yet
> - if (warequeues1[i]->get_max_fill() <= 1) {
> + if (queue->get_max_fill() <= 1) {
> continue;
> }
>
> // now modifying max_fill of armors and weapons
> for (std::string pattern : armors_and_weapons) {
>
> - if
> (tribe_->get_ware_descr(warequeues1[i]->get_index())->name().find(pattern) !=
> + if
> (tribe_->get_ware_descr(queue->get_index())->name().find(pattern) !=
> std::string::npos) {
> - if (warequeues1[i]->get_max_fill() > 1) {
> -
> game().send_player_set_input_max_fill(*ts, warequeues1[i]->get_index(),
> wwWARE, 1);
> + if (queue->get_max_fill() > 1) {
> +
> game().send_player_set_input_max_fill(*ts, queue->get_index(), wwWARE, 1);
> continue;
> }
> }
> @@ -518,11 +522,13 @@
> // minutes)
> // we can accept also shortage up to 3
> int32_t shortage = 0;
> - std::vector<WaresQueue*> const warequeues2 =
> tso.site->warequeues();
> - nr_warequeues = warequeues2.size();
> - for (size_t i = 0; i < nr_warequeues; ++i) {
> - if
> (tso.bo->substitute_inputs.count(warequeues2[i]->get_index()) > 0) {
> - filled += warequeues2[i]->get_filled();
> + std::vector<InputQueue*> const inputqueues2 =
> tso.site->inputqueues();
> + for (InputQueue *queue : inputqueues2) {
How about:
for (InputQueue *queue : tso.site->inputqueues()) {
and lose the extra variable?
> + if (queue->get_type() != wwWARE) {
> + continue;
> + }
> + if (tso.bo->substitute_inputs.count(queue->get_index())
> > 0) {
> + filled += queue->get_filled();
> }
> }
> if (filled < 5) {
> @@ -530,12 +536,15 @@
> }
>
> // checking non subsitutes
> - for (size_t i = 0; i < nr_warequeues; ++i) {
> - if
> (tso.bo->substitute_inputs.count(warequeues2[i]->get_index()) == 0) {
> + for (InputQueue *queue : inputqueues2) {
How about:
for (InputQueue *queue : tso.site->inputqueues()) {
and lose the extra variable?
> + if (queue->get_type() != wwWARE) {
> + continue;
> + }
> + if (tso.bo->substitute_inputs.count(queue->get_index())
> == 0) {
> const uint32_t required_amount =
> - (warequeues2[i]->get_max_fill() < 5) ?
> warequeues2[i]->get_max_fill() : 5;
> - if (warequeues2[i]->get_filled() <
> required_amount) {
> - shortage += required_amount -
> warequeues2[i]->get_filled();
> + (queue->get_max_fill() < 5) ?
> queue->get_max_fill() : 5;
> + if (queue->get_filled() < required_amount) {
> + shortage += required_amount -
> queue->get_filled();
> }
> }
> }
>
> === modified file 'src/logic/map_objects/tribes/constructionsite.cc'
> --- src/logic/map_objects/tribes/constructionsite.cc 2016-12-05 09:24:10
> +0000
> +++ src/logic/map_objects/tribes/constructionsite.cc 2017-01-22 16:41:24
> +0000
> @@ -80,14 +80,20 @@
> Access to the wares queues by id
> =======
> */
> -WaresQueue& ConstructionSite::waresqueue(DescriptionIndex const wi) {
> +InputQueue& ConstructionSite::inputqueue(DescriptionIndex const wi,
> WareWorker const type) {
> + // There are no worker queues here
> + // Hopefully, our construction sites are save enough not to kill workers
save -> safe
> + if (type != wwWARE) {
> + throw wexception("%s (%u) (building %s) has no WorkersQueues",
> descr().name().c_str(),
> + serial(), building_->name().c_str());
> + }
> for (WaresQueue* ware : wares_) {
> if (ware->get_index() == wi) {
> return *ware;
> }
> }
> throw wexception("%s (%u) (building %s) has no WaresQueue for %u",
> descr().name().c_str(),
> - serial(), building_->name().c_str(), wi);
> + serial(), building_->name().c_str(),
> wi);
> }
>
> /*
>
> === modified file 'src/logic/map_objects/tribes/production_program.cc'
> --- src/logic/map_objects/tribes/production_program.cc 2017-01-21
> 21:10:21 +0000
> +++ src/logic/map_objects/tribes/production_program.cc 2017-01-22
> 16:41:24 +0000
> @@ -804,39 +803,38 @@
> }
>
> void ProductionProgram::ActConsume::execute(Game& game, ProductionSite& ps)
> const {
> - std::vector<WaresQueue*> const warequeues = ps.warequeues();
> - std::vector<WorkersQueue*> const workerqueues = ps.workerqueues();
> - std::vector<uint8_t> consumption_quantities_wares(warequeues.size(), 0);
> - std::vector<uint8_t>
> consumption_quantities_workers(workerqueues.size(), 0);
> + std::vector<InputQueue*> const inputqueues = ps.inputqueues();
> + std::vector<uint8_t> consumption_quantities(inputqueues.size(), 0);
>
> Groups l_groups = consumed_wares_workers_; // make a copy for local
> modification
>
> // Iterate over all input queues and see how much we should consume
> from
> // each of them.
> bool found;
> - for (size_t i = 0; i < warequeues.size(); ++i) {
> - DescriptionIndex const ware_type = warequeues[i]->get_index();
> - uint8_t nr_available = warequeues[i]->get_filled();
> - consumption_quantities_wares[i] = 0;
> + for (size_t i = 0; i < inputqueues.size(); ++i) {
> + DescriptionIndex const input_index =
> inputqueues[i]->get_index();
> + WareWorker const input_type = inputqueues[i]->get_type();
> + uint8_t nr_available = inputqueues[i]->get_filled();
> + consumption_quantities[i] = 0;
>
> // Iterate over all consume groups and see if they want us to
> consume
> // any thing from the currently considered input queue.
> for (Groups::iterator it = l_groups.begin(); it !=
> l_groups.end();) {
> found = false;
> - for (auto ware_it = it->first.begin(); ware_it !=
> it->first.end(); ware_it++) {
> - if (ware_it->first == ware_type &&
> ware_it->second == wwWARE) {
> + for (auto input_it = it->first.begin(); input_it !=
> it->first.end(); input_it++) {
Is this iterator used to erase or insert elements? If not, a range-based for
loop would be nice :)
for (const auto& input : first) {
> + if (input_it->first == input_index &&
> input_it->second == input_type) {
> found = true;
> if (it->second <= nr_available) {
> // There are enough wares of
> the currently considered type
> // to fulfill the requirements
> of the current group. We can
> // therefore erase the group.
> - consumption_quantities_wares[i]
> += it->second;
> + consumption_quantities[i] +=
> it->second;
> nr_available -= it->second;
> it = l_groups.erase(it);
> // No increment here, erase
> moved next element to the position
> // pointed to by it.
> } else {
> - consumption_quantities_wares[i]
> += nr_available;
> + consumption_quantities[i] +=
> nr_available;
> it->second -= nr_available;
> ++it; // Now check if the
> next group includes this ware type.
> }
> @@ -934,19 +904,15 @@
> ps.set_production_result(result_string);
> return ps.program_end(game, Failed);
> } else { // we fulfilled all consumption requirements
> - for (size_t i = 0; i < warequeues.size(); ++i) {
> - if (uint8_t const q = consumption_quantities_wares[i]) {
> - assert(q <= warequeues[i]->get_filled());
> -
> warequeues[i]->set_filled(warequeues[i]->get_filled() - q);
> + for (size_t i = 0; i < inputqueues.size(); ++i) {
> + if (uint8_t const q = consumption_quantities[i]) {
> + assert(q <= inputqueues[i]->get_filled());
> +
> inputqueues[i]->set_filled(inputqueues[i]->get_filled() - q);
>
> - // Update consumption statistics
> -
> ps.owner().ware_consumed(warequeues[i]->get_index(), q);
> - }
> - }
> - for (size_t i = 0; i < workerqueues.size(); ++i) {
> - if (uint8_t const q =
> consumption_quantities_workers[i]) {
> - assert(q <= workerqueues[i]->get_filled());
> -
> workerqueues[i]->set_filled(workerqueues[i]->get_filled() - q);
> + // Update consumption statistic
statistic => statistics
> + if (inputqueues[i]->get_type() == wwWARE) {
> +
> ps.owner().ware_consumed(inputqueues[i]->get_index(), q);
> + }
> }
> }
> return ps.program_step(game);
>
> === modified file 'src/map_io/map_buildingdata_packet.cc'
> --- src/map_io/map_buildingdata_packet.cc 2017-01-06 09:00:11 +0000
> +++ src/map_io/map_buildingdata_packet.cc 2017-01-22 16:41:24 +0000
> @@ -1140,16 +1140,27 @@
> fw.unsigned_8(productionsite.program_timer_);
> fw.signed_32(productionsite.program_time_);
>
> - const uint16_t input_queues_size =
> productionsite.input_ware_queues_.size();
> - fw.unsigned_16(input_queues_size);
> - for (uint16_t i = 0; i < input_queues_size; ++i) {
> - productionsite.input_ware_queues_[i]->write(fw, game, mos);
> + // Get number of ware queues. Not very pretty but avoids changing the
> save file format
> + uint16_t input_ware_queues_size = 0;
> + for (InputQueue *iq : productionsite.inputqueues()) {
> + if (iq->get_type() == wwWARE) {
> + input_ware_queues_size++;
> + }
> + }
> + // Write count of ware queues and ware queues
ware queues and ware queues?
> + fw.unsigned_16(input_ware_queues_size);
> + for (InputQueue *iq : productionsite.inputqueues()) {
> + if (iq->get_type() == wwWARE) {
> + iq->write(fw, game, mos);
> + }
> }
>
> - const uint16_t input_worker_queues_size =
> productionsite.input_worker_queues_.size();
> - fw.unsigned_16(input_worker_queues_size);
> - for (uint16_t i = 0; i < input_worker_queues_size; ++i) {
> - productionsite.input_worker_queues_[i]->write(fw, game, mos);
> + // Same for worker queues
> + fw.unsigned_16(productionsite.input_queues_.size() -
> input_ware_queues_size);
> + for (InputQueue *iq : productionsite.inputqueues()) {
> + if (iq->get_type() == wwWORKER) {
> + iq->write(fw, game, mos);
> + }
> }
>
> const uint16_t statistics_size = productionsite.statistics_.size();
--
https://code.launchpad.net/~widelands-dev/widelands/refactoring-input-queue/+merge/315313
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/refactoring-input-queue.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
