Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-03 Thread GunChleoc 
All clear :)

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/gcc531/+merge/293478
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/gcc531.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-03 Thread Klaus Halfmann 
Review: Approve

OK, for me with _my_ change, but gcc may complain again?
-- 
https://code.launchpad.net/~widelands-dev/widelands/gcc531/+merge/293478
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/gcc531.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread GunChleoc 
I didn't check the datatype ;)

Diff comments:

> 
> === modified file 'src/map_io/map_players_view_packet.cc'
> --- src/map_io/map_players_view_packet.cc 2016-02-16 10:27:23 +
> +++ src/map_io/map_players_view_packet.cc 2016-05-01 10:05:57 +
> @@ -991,12 +991,12 @@
>   }
>  
>   //  edges
> - if (!bl_seen & (f_everseen | 
> bl_everseen))
> + if (!(bl_seen) & (f_everseen | 
> bl_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_sw());
> - if (!br_seen & (f_everseen | 
> br_everseen))
> + if (!(br_seen) & (f_everseen | 
> br_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_se());
> - if (!r_seen & (f_everseen |  
> r_everseen))
> - 
> roads_file.unsigned_8(f_player_field.road_e ());
> + if (!(r_seen) & (f_everseen | 
> r_everseen))
> + 
> roads_file.unsigned_8(f_player_field.road_e());

I didn't check the datatype - I just assumed that we were having some bitmasks 
here. It doesn't matter now, since I have managed to fix it :)

>   }
>  
>   //  geologic survey


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

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread Miroslav Remák 
Replied to a diff comment.

Diff comments:

> 
> === modified file 'src/map_io/map_players_view_packet.cc'
> --- src/map_io/map_players_view_packet.cc 2016-02-16 10:27:23 +
> +++ src/map_io/map_players_view_packet.cc 2016-05-01 10:05:57 +
> @@ -991,12 +991,12 @@
>   }
>  
>   //  edges
> - if (!bl_seen & (f_everseen | 
> bl_everseen))
> + if (!(bl_seen) & (f_everseen | 
> bl_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_sw());
> - if (!br_seen & (f_everseen | 
> br_everseen))
> + if (!(br_seen) & (f_everseen | 
> br_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_se());
> - if (!r_seen & (f_everseen |  
> r_everseen))
> - 
> roads_file.unsigned_8(f_player_field.road_e ());
> + if (!(r_seen) & (f_everseen | 
> r_everseen))
> + 
> roads_file.unsigned_8(f_player_field.road_e());

Care to elaborate on why using logical operations here would change semantics? 
Bools can only hold 0 or 1, and all of the operand variables are bools, so the 
results should be the same for both bitwise and logical operations.

>   }
>  
>   //  geologic survey


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

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread GunChleoc 
Might be Ubuntu-sepecific numbering.
-- 
https://code.launchpad.net/~widelands-dev/widelands/gcc531/+merge/293478
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/gcc531.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread GunChleoc 
That's what my gcc --version says.
-- 
https://code.launchpad.net/~widelands-dev/widelands/gcc531/+merge/293478
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/gcc531.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread Tino 
GCC 5.3.1 ?
It is not listed on https://gcc.gnu.org/ ...
-- 
https://code.launchpad.net/~widelands-dev/widelands/gcc531/+merge/293478
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/gcc531.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-02 Thread GunChleoc 
Replied to the comments.

Diff comments:

> === modified file 'src/logic/queue_cmd_factory.cc'
> --- src/logic/queue_cmd_factory.cc2016-01-18 05:12:51 +
> +++ src/logic/queue_cmd_factory.cc2016-05-01 10:05:57 +
> @@ -112,7 +112,7 @@
>   case QueueCommandTypes::kReplaySyncRead:
>   case QueueCommandTypes::kReplayEnd:
>   case QueueCommandTypes::kNone:
> - throw wexception("Unknown Queue_Cmd_Id in file: %u", id);
> + throw wexception("Unknown Queue_Cmd_Id in file: %u", 
> static_cast(id));
>   }

The Travis clang builds are clean here - could you give me the text of the 
warning, please?

>   NEVER_HERE();
>  }
> 
> === modified file 'src/map_io/map_players_view_packet.cc'
> --- src/map_io/map_players_view_packet.cc 2016-02-16 10:27:23 +
> +++ src/map_io/map_players_view_packet.cc 2016-05-01 10:05:57 +
> @@ -991,12 +991,12 @@
>   }
>  
>   //  edges
> - if (!bl_seen & (f_everseen | 
> bl_everseen))
> + if (!(bl_seen) & (f_everseen | 
> bl_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_sw());
> - if (!br_seen & (f_everseen | 
> br_everseen))
> + if (!(br_seen) & (f_everseen | 
> br_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_se());
> - if (!r_seen & (f_everseen |  
> r_everseen))
> - 
> roads_file.unsigned_8(f_player_field.road_e ());
> + if (!(r_seen) & (f_everseen | 
> r_everseen))
> + 
> roads_file.unsigned_8(f_player_field.road_e());

That's a false positive from our own codecheck rules - I have to see about 
fixing that.

Changing bitwise operators to non-bitwise operators will completely change the 
semantics of the function - don't change it.

http://www.cprogramming.com/tutorial/bitwise_operators.html

>   }
>  
>   //  geologic survey


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

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/gcc531 into lp:widelands

2016-05-01 Thread Klaus Halfmann 
Review: Needs Information codereview, compile

I now get some (new?) clang warning, see inline comments.
Shall I commit the change?

Diff comments:

> === modified file 'src/logic/queue_cmd_factory.cc'
> --- src/logic/queue_cmd_factory.cc2016-01-18 05:12:51 +
> +++ src/logic/queue_cmd_factory.cc2016-05-01 10:05:57 +
> @@ -112,7 +112,7 @@
>   case QueueCommandTypes::kReplaySyncRead:
>   case QueueCommandTypes::kReplayEnd:
>   case QueueCommandTypes::kNone:
> - throw wexception("Unknown Queue_Cmd_Id in file: %u", id);
> + throw wexception("Unknown Queue_Cmd_Id in file: %u", 
> static_cast(id));
>   }

Mhh, there only really good way to ourput an enum is some kind of toString() 
method,
But I do not know if C++ has some defaults here :-(. But this is an exceptional 
case :-)

>   NEVER_HERE();
>  }
> 
> === modified file 'src/map_io/map_players_view_packet.cc'
> --- src/map_io/map_players_view_packet.cc 2016-02-16 10:27:23 +
> +++ src/map_io/map_players_view_packet.cc 2016-05-01 10:05:57 +
> @@ -991,12 +991,12 @@
>   }
>  
>   //  edges
> - if (!bl_seen & (f_everseen | 
> bl_everseen))
> + if (!(bl_seen) & (f_everseen | 
> bl_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_sw());
> - if (!br_seen & (f_everseen | 
> br_everseen))
> + if (!(br_seen) & (f_everseen | 
> br_everseen))
>   
> roads_file.unsigned_8(f_player_field.road_se());
> - if (!r_seen & (f_everseen |  
> r_everseen))
> - 
> roads_file.unsigned_8(f_player_field.road_e ());
> + if (!(r_seen) & (f_everseen | 
> r_everseen))
> + 
> roads_file.unsigned_8(f_player_field.road_e());

clang now complains:
src/map_io/map_players_view_packet.cc:994: Old C-Style cast. Change to 
static_cast(var) or similar!
src/map_io/map_players_view_packet.cc:996: Old C-Style cast. Change to 
static_cast(var) or similar!
src/map_io/map_players_view_packet.cc:998: Old C-Style cast. Change to 
static_cast(var) or similar!

Mhh, what does gcc complain about? As all these are boolean this code might be 
better with && and ||.
Shall I check in the change?

>   }
>  
>   //  geologic survey


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

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp