widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve

Code LGTM - 2 tiny nits. Not tested.

Diff comments:

> 
> === modified file 'src/logic/cmd_luacoroutine.cc'
> --- src/logic/cmd_luacoroutine.cc	2017-01-25 18:55:59 +0000
> +++ src/logic/cmd_luacoroutine.cc	2017-07-05 21:50:35 +0000
> @@ -51,11 +53,11 @@
>  		log("%s\n", e.what());
>  		log("Send message to all players and pause game\n");
>  		for (int i = 1; i <= game.map().get_nrplayers(); i++) {
> -			Widelands::Message& msg = *new Widelands::Message(
> +			std::unique_ptr<Message >msg(new Widelands::Message(

<Message >msg => <Message> msg. Does clang-format fix this?

>  			   Message::Type::kGameLogic, game.get_gametime(), "Coroutine",
>  			   "images/ui_basic/menu_help.png", "Lua Coroutine Failed",
> -			   (boost::format("<rt><p font-size=12>%s</p></rt>") % e.what()).str());
> -			game.player(i).add_message(game, msg, true);
> +			   (boost::format("<rt><p font-size=12>%s</p></rt>") % e.what()).str()));
> +			game.player(i).add_message(game, std::move(msg), true);
>  		}
>  		game.game_controller()->set_desired_speed(0);
>  	}
> 
> === modified file 'src/logic/message_queue.h'
> --- src/logic/message_queue.h	2017-01-25 18:55:59 +0000
> +++ src/logic/message_queue.h	2017-07-05 21:50:35 +0000
> @@ -39,29 +42,27 @@
>  	}                                                              //  C++0x:
>  
>  	~MessageQueue() {
> -		while (size()) {
> -			delete begin()->second;
> -			erase(begin());
> -		}
>  	}
>  
>  	//  Make some selected inherited members public.
> -	MessageQueue::const_iterator begin() const {
> -		return std::map<MessageId, Message*>::begin();
> -	}
> -	MessageQueue::const_iterator end() const {
> -		return std::map<MessageId, Message*>::end();
> -	}
> -	size_type count(uint32_t const i) const {
> +	//  TODO(sirver): This is weird design. Instead pass out a const ref& to
> +	//  'messages_'?
> +	MessageMap::const_iterator begin() const {
> +		return messages_.begin();
> +	}
> +	MessageMap::const_iterator end() const {
> +		return messages_.end();
> +	}
> +	size_t count(uint32_t const i) const {
>  		assert_counts();
> -		return std::map<MessageId, Message*>::count(MessageId(i));
> +		return messages_.count(MessageId(i));
>  	}
>  
>  	/// \returns a pointer to the message if it exists, otherwise 0.

otherwise nullptr

>  	Message const* operator[](const MessageId& id) const {
>  		assert_counts();
> -		MessageQueue::const_iterator const it = find(MessageId(id));
> -		return it != end() ? it->second : nullptr;
> +		const auto it = messages_.find(MessageId(id));
> +		return it != end() ? it->second.get() : nullptr;
>  	}
>  
>  	/// \returns the number of messages with the given status.


-- 
https://code.launchpad.net/~widelands-dev/widelands/00_private_inheritance/+merge/326886
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/00_private_inheritance.