widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

notabilis is right and there is one other bug in this change. I added inline comments to explain what is going on.

Diff comments:

> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc	2016-11-19 18:25:49 +0000
> +++ src/ai/defaultai.cc	2016-11-20 10:15:14 +0000
> @@ -2997,8 +2997,7 @@
>  	if (RoadCandidates.get_winner(&winner_hash, (gametime % 4 > 0) ? 1 : 2)) {
>  		const Widelands::Coords target_coords = Coords::unhash(winner_hash);
>  		Path& path = *new Path();
> -		const int32_t pathcost = map.findpath(flag.get_position(), target_coords, 0, path, check);
> -		assert(pathcost >= 0);
> +		assert(map.findpath(flag.get_position(), target_coords, 0, path, check) >= 0);

this is broken! the assert will not be in release builds, so map.findpath will never be called and this always returns an empty path.

You have to use something like:
#ifndef NDEBUG
const int32_t pathcost = map.findpath(flag.get_position(), target_coords, 0, path, check);
assert(...)
#else
map.findpath(flag.get_position(), target_coords, 0, path, check);
#endif

>  		game().send_player_build_road(player_number(), path);
>  		return true;
>  	}
> 
> === modified file 'src/network/network.h'
> --- src/network/network.h	2016-08-04 15:49:05 +0000
> +++ src/network/network.h	2016-11-20 10:15:14 +0000
> @@ -175,23 +176,16 @@
>   */
>  struct ProtocolException : public std::exception {
>  	explicit ProtocolException(uint8_t code) {
> -		what_ = code;
> +		what_ = boost::lexical_cast<std::string>(static_cast<unsigned int>(code)).c_str();

this is also broken: This creates a new std::string returned by lexical_cast, then takes a pointer to its internal data (and keeps that in what_) and then delete the string again. Change what_ to be a std::string and return what_.cstr() in what()

>  	}
>  
> -	/// do NOT use!!! This exception shall only return the command number of the received message
> -	/// via \ref ProtocolException:number()
> +	/// \returns the command number of the received message
>  	const char* what() const noexcept override {
> -		NEVER_HERE();
> -	}
> -
> -	/// \returns the command number of the received message
> -	virtual int number() const {
>  		return what_;
>  	}
>  
>  private:
> -	// no uint8_t, as lexical_cast does not support that format
> -	int what_;
> +	const char* what_;
>  };
>  
>  #endif  // end of include guard: WL_NETWORK_NETWORK_H


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