widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.