Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands
Looks good to me, let's have it! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-better-syncstreams. ___ 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/bug-1815277-persistence-memory-leak into lp:widelands
I got 2 leak entries previously, going through the same line in persistence.cc. Now I'm only getting 1. Ma C is not good enough for me to touch Eris, but if you can fix it, we could contribute our fix to the eris project as well. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1815277-persistence-memory-leak/+merge/362963 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands. ___ 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/bug-1810062-territorial-calculations into lp:widelands
Duh, I overlooked the n. Your solution sounds good to me. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1810062-territorial-calculations into lp:widelands
Continuous integration builds have changed state: Travis build 4461. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/491840111. Appveyor build 4249. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1810062_territorial_calculations-4249. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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/bug-1810062-territorial-calculations into lp:widelands
Ok I tested it and it is a completely logical behaviour: 1. the assert tests the size of a field object to ensure the preprocessor command to pack this struct was successful 2. before my changes this object had 10 bytes + 2 times the standard address width (sizeof (void*)) 3. I added a property as uint8 which needs another byte. so this results in 11 bytes + 2 times sizeof(void*) 4. the winows assert allowed for up to 11 bytes and did not complain, the other OS assert was checking the 10 bytes and complained logically. so the solution (if we want to have this change) is indeed to adapt the assert to the new size of the field struct. I uploaded a new revision with the relevant changes. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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/bug-1815277-persistence-memory-leak into lp:widelands
I was able to reproduce the memory leak of the bug report but I wasn't able to observe a change by this branch. Both times I loaded the same autosave single player game of Autocrat and got the same leak-message (except for the memory addresses). Should something change in the output? I am also unsure whether your change really changes anything. As far as I know, a reference is basically another way to write a pointer, so no memory release is done by your code. But maybe I am wrong about this. >From the call stack and some digging through the eris code (that I don't >really understand) it seems to me as if the leak is / might be in the >u_closure() function deep within the eris code. Do we fix third-party leaks or >do we just ignore them? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1815277-persistence-memory-leak/+merge/362963 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/coroutine-error-collectors into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/coroutine-error-collectors into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/coroutine-error-collectors/+merge/362956 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/coroutine-error-collectors. ___ 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/bug-1810062-territorial-calculations into lp:widelands
the check for WIN32 is an ifndef check so in fact the check for Windows is less strict. My code adds a new property to the field struct so I believe it is increased in size and therefore the check for all other Systems than Windows fail due to the more restrictive size check. As these asserts are checking the size of field I can't see the reason why the assert is different for different OS. As I haven't a Linux machine ready I'll try it the other way round by lowering the windows part as well and see if MSYS2 environment complains as well. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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/coroutine-error-collectors into lp:widelands
Review: Approve Tested and working :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/coroutine-error-collectors/+merge/362956 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/coroutine-error-collectors. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands
Continuous integration builds have changed state: Travis build 4455. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/491524355. Appveyor build 4243. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1815277_persistence_memory_leak-4243. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1815277-persistence-memory-leak/+merge/362963 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands. ___ 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/bug-1810062-territorial-calculations into lp:widelands
The check under Windows is more strict, so unifying the check will not make the assert error go away. The roper fix is to understand what the problem is - I expect that there is a reason that somebody put that assert there and that there is a bug in the new code. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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/bug-1810062-territorial-calculations into lp:widelands
The check that is failing is checking whether a field instance is tightly packed. (whatever that means) However the reason for Linux failing while appveyor / MSYS2 not failing is that this check is defined different for a WIN32 system as for the other systems. The reason for this is unknown to me. So I would propose to fix line 261 ff. of field.h and doing the check equally for all systems (i.e. deleting the if loop) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/scotty_the_scout into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/scotty_the_scout into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/scotty_the_scout/+merge/362943 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/scotty_the_scout. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/string-fixes into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/string-fixes into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/string-fixes/+merge/362944 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/string-fixes. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-memleak-net-ui/+merge/362945 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands. ___ 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/string-fixes into lp:widelands
:-( Sorry GunChleoc. I'm going to relink them later. -- https://code.launchpad.net/~widelands-dev/widelands/string-fixes/+merge/362944 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/string-fixes. ___ 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/coroutine-error-collectors into lp:widelands
Set it to true. Thanks kaputtnik. -- https://code.launchpad.net/~widelands-dev/widelands/coroutine-error-collectors/+merge/362956 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/coroutine-error-collectors into lp:widelands. ___ 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/bug-1810062-territorial-calculations into lp:widelands
I didn't know about this as I use a MSYS2 Environment (like Appveyor) where it compiled and worked. Unfortunately my C++ knowledge is very limited, so I am not sure how to fix this. If anyone of you knows what is going on there feel free to correct it. I will try to do so as well tonight. I believe this assert is to ensure minimal ressources consumption of a field object cause there are so many of them. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations. ___ 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/bug-memleak-net-ui into lp:widelands
Memory leak and fix confirmed. Code LGTM :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-memleak-net-ui/+merge/362945 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands. ___ 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/coroutine-error-collectors into lp:widelands
I also think that it should pop up. -- https://code.launchpad.net/~widelands-dev/widelands/coroutine-error-collectors/+merge/362956 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/coroutine-error-collectors into lp:widelands. ___ 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into
lp:widelands.
Commit message:
Fix a memory leak in persistence.cc
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1815277 in widelands: "Memory leak in persistence.cc while loading game"
https://bugs.launchpad.net/widelands/+bug/1815277
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1815277-persistence-memory-leak/+merge/362963
There is still a second leak on the same line that I didn't manage to fix.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into
lp:widelands.
=== modified file 'src/scripting/persistence.cc'
--- src/scripting/persistence.cc 2018-04-07 16:59:00 +
+++ src/scripting/persistence.cc 2019-02-11 08:25:08 +
@@ -48,10 +48,10 @@
}
const char* LuaReader(lua_State* /* L */, void* userdata, size_t* bytes_read) {
- LuaReaderHelper* helper = static_cast(userdata);
+ const LuaReaderHelper& helper = *static_cast(userdata);
- *bytes_read = helper->data_len;
- return helper->data.get();
+ *bytes_read = helper.data_len;
+ return helper.data.get();
}
} // namespace
___
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/scotty_the_scout into lp:widelands
Thanks! @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/scotty_the_scout/+merge/362943 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/scotty_the_scout. ___ 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/string-fixes into lp:widelands
The bugs are related to the branch because this is a permanent branch that I keep recycling. Just like the bugs that are linked to trunk, they should have remained linked. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/string-fixes/+merge/362944 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/string-fixes. ___ 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
