Review: Approve
Refactored a bit, fixed some bugs and merged. I only fixed the bugs that the
tests found - this change touched quite a bit of logic, so it could be that
there are more.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developer
The proposal to merge lp:~widelands-dev/widelands/bug-1341081 into lp:widelands
has been updated.
Status: Needs review => Merged
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
--
https://code.launchpad.net/~widelands-dev/widelands/bug-13
Guess who forgot to push :P
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
___
Mailing list: https://launchpad.net/~widela
Review: Needs Fixing
This needs trunk merging really bad.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
___
Mailing list
Review: Resubmit
I fixed the line and cleaned out the build directory and the segfault is gone.
Thanks!
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
_
GunChleoc,
it failed to compile here:
[ 82%] Building CXX object
src/map_io/CMakeFiles/map_io.dir/widelands_map_object_packet.cc.o
In file included from
/var/widelands/BZR/bug-1341081/bug-1341081/src/map_io/widelands_map_object_packet.cc:22:0:
/var/widelands/BZR/bug-1341081/bug-1341081/src/map_
OK, next try, but I need somebody to help me hunt down a segfault. I have
described the problem in my commit message
http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7121
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands
Review: Needs Fixing
1) Is strange, but makes sense. I do not fully understand why that is needed
now and wasn't before, but meh.
2) Dangerous assumption. The speed of a dynamic_cast() depends hugely on many
factors. String comparison is always slow. See
http://stackoverflow.com/questions/9778
Review: Resubmit
This is ready for review again.
2 things I'd like to point out:
1. I had to change the visibility of a class member for the code to compile -
check the NOCOM here:
http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1341081/revision/7113#src/logic/carrier.h
Since it'S st
Thanks for the info.
I am looking forward to Ninja :D
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
___
Mailing list: ht
Yes, we are using tabs. Personally, I'd much rather switch to spaces and
reformat all code with clang-format, which would then be the definition of
'correct'. I got push back the last two times I tried to establish this though.
> The codecheck warnings are hidden behind the "check to see if code
> Also, your editor formats source code quite differently to the codebase, so
> there are a lot of style checker warnings. Don't you get style warnings during
> compile?
It's a matter of setting up the editor to confirm with our code formatting
standards - which I can't find a reference for. I do
I added a bunch of NOCOM(#codereview). You do not implement some methods (see
the codereview comments). The code I pushed still doesn't compile, but it
should give you an idea of what is missing.
Also, your editor formats source code quite differently to the codebase, so
there are a lot of styl
I tried and it doesn't compile - see comment. I don't know why.
Diff comments:
> === modified file 'campaigns/atl01.wmf/scripting/init.lua'
> --- campaigns/atl01.wmf/scripting/init.lua2014-07-17 15:26:32 +
> +++ campaigns/atl01.wmf/scripting/init.lua2014-07-19 09:00:26 +
> @@ -324
Review: Needs Fixing
Diff comments:
> === modified file 'campaigns/atl01.wmf/scripting/init.lua'
> --- campaigns/atl01.wmf/scripting/init.lua2014-07-17 15:26:32 +
> +++ campaigns/atl01.wmf/scripting/init.lua2014-07-19 09:00:26 +
> @@ -324,7 +324,7 @@
>if not f.immovable
I reinstated the anonymour namespace for g_Battle_Descr. The others are used in
other classes and thus need to be visible.
Diff comments:
> === modified file 'campaigns/atl01.wmf/scripting/init.lua'
> --- campaigns/atl01.wmf/scripting/init.lua2014-07-15 10:02:22 +
> +++ campaigns/atl01.w
There is also a diff comment buried in my last comment.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1341081.
___
Mailing list: h
Review: Needs Fixing
> This avoids having to get the tribes' Immovable_Descr through the Lua
> interface. If you think this is a problem, I can have a go at it though.
> Otherwise, ready for merging.
I do not think this is a problem. Wrapping those too might make sense in the
future anyways th
GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1341081 into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1341081 in widelands: "Building help: clean up type() and
MapObject::type_name() "
https://bugs.launchpad.net/widelands/+bug/
19 matches
Mail list logo
