Time Nick Message 08:27 Krock nerzhul, so shall I add the comments or whitelist the file? for me it looks like you're in favour for the former, but paramat disagrees on that 10:35 red-001 #7484 10:35 ShadowBot https://github.com/minetest/minetest/issues/7484 -- Fix buffer overrun in SRP by red-001 10:36 red-001 might be a good idea to write unit tests for SRP 11:00 Krock sfan5, #7482 looks good. Is it tested yet? 11:00 ShadowBot https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 11:00 sfan5 haven't tested yet 11:01 Krock okay 11:01 sfan5 if you have the android build stuff installed, it should be quick to test 11:01 sfan5 otherwise i can do it later today 11:01 Krock I'd have to set it up first. Testing is after all not much of a thing as soon building works 15:07 sfan5 Krock: tested it now, works. 15:10 Krock Thanks. LGTM. 15:11 red-001 anyone have time to look at 7478, it's a one line fix 15:11 red-001 *? 15:11 red-001 *7484 15:14 rubenwardy #7484 15:14 ShadowBot https://github.com/minetest/minetest/issues/7484 -- Fix buffer overrun in SRP by red-001 15:49 Krock red-001, shouldn't that be g_rand_buff[g_rand_idx] instead of &g_rand_buff[g_rand_idx] ? 15:50 Krock oh. memcpy requires a pointer, by indexing the array it's not a pointer. nvm 15:51 sfan5 hm would be nice if gcc had a warning for that 15:52 sfan5 adding an offset onto an array pointer is quite weird, but i can see why gcc doesn't warn about it 15:53 Krock well, in murmur hash it's the same to calculate the end address 15:53 rubenwardy isn't p[a] equiv to p + a*sizeof(*p) 15:54 sfan5 it is 15:55 sfan5 Krock: what I mean is that if x is known to be an array on either the stack or in .(ro)data, adding something to &x is most certainly a mistake 15:57 sfan5 why does srp use this rand buf thing anyway? just get randomness on demand 16:05 paramat i have pre-approval to do a little intuitive reordering of lua_api.txt as described here https://github.com/minetest/minetest/issues/7404#issuecomment-399678558 please state any objections now before i work on it thanks 16:46 paramat the linter is being a pain again https://github.com/minetest/minetest/pull/7483#issuecomment-399788100 it confuses contributors and makes them think they are making codestyle mistakes 16:47 Krock Lint is not wrong with most of the suggestions. the old code is just formatted poorly 16:48 paramat no 16:48 paramat that's incorrrect 16:49 paramat it doesn't know MT codestyle at all so much of what it reports is contrary to our codestyle 16:50 Krock we already had this topic previously. not just once. Lint offers too few parameters to make it work correctly in all cases for our rules 16:50 rubenwardy most of that in the PR is correct 16:50 paramat maybe around 50%, varies according to file. in mapgen files it makes about 50 false reports, but that's the worst case 16:50 rubenwardy the constructor args is questionable 16:51 sfan5 >(gui::IGUIEditBox *)hovered) 16:51 sfan5 this looks pretty weird 16:52 paramat yes, it's understandable it can't know MT codestyle well, i don't expect it to 16:52 sfan5 no it's not understandable, if your linter doesn't support your code style you switch to a new linter 16:52 paramat which is why it's debateable it is worth having. it's useful, but also a huge pain 16:53 Krock We should consider to program our own Linter with AI and blockchain technique /s 16:54 Krock well, what options are there which are supported by the dev tools on GitHub? 16:54 sfan5 anyway, IMO it would be best to fix the code style in that very PR 16:54 paramat it tends to teach contributers wrong codestyle, and wastes time because we have to then point out where it is wrong 16:54 sfan5 Krock: github integration does not really matter, it runs on travis anyway 16:57 Krock sfan5, in that case it's a question how much we can customize Travis to make use of another tool 17:01 Krock Updated #7468 17:01 ShadowBot https://github.com/minetest/minetest/issues/7468 -- [Don't squash me!] Rename CSM flavours to CSM restrictions by SmallJoker 17:01 paramat the double-indents it requests are silly 17:01 paramat 1 tab indent for a split line is fine and reduces indentation 17:02 paramat also of course a reviewer has to review codestyle anyway and would catch most of what linter reports, so it doesn't save time 17:03 sfan5 >[reviewer] would catch most of what linter reports 17:03 sfan5 good joke 17:04 Krock we cannot rely on human control with large PRs, where it's more important that there are no problems/bugs than how the code is formatted 17:04 paramat i'm looking at the lint report now. it is complaining about formatting we have used for readability, like vertical alignment 17:04 sfan5 also, 1 tab indent for a split line is not what our code style says 17:04 paramat overall that report is about 50% correct 17:07 paramat our codestyle doesn't specify indents, however 1 tab indent is fine 17:07 paramat more just shifts everything too far in some situations which causes more line splitting 17:08 paramat code formating for readability is very important, we can't make code less readable just for a bad linter 17:09 paramat so, i think we need a better one, or none 17:09 paramat the more recent clang-tidy checks are good though 17:09 sfan5 two tabs to split lines is what we have everywhere else though 17:09 paramat no 17:10 paramat it's a mix 17:10 sfan5 and it's also part of the rules for conditionals 17:10 paramat yes, i'm not talking about conditionals 17:10 sfan5 I know 17:11 paramat it's needed where differentiation is needed, not everywhere 17:11 red-001 sfan5, idk maybe someone thought getting the data ever single time was a preformence issue 17:12 sfan5 red-001: probably, but that's premature optimization 17:12 red-001 the whole file has a very different coding style to the rest of minetest 17:12 red-001 feels like I'm reading kernal code 17:13 Krock maybe because we use the kernel code style guidelines? 17:13 paramat yes we do, for anyhting not specified in our own guidelines 17:14 paramat *anything 17:14 rubenwardy kernel code has way more gotos 17:14 rubenwardy like, if a function doesn't have at least one goto it's not allowed 17:14 red-001 some gotos > no gotos 17:15 * rubenwardy added his first feature to the Linux kernel 17:15 rubenwardy not mainstream, obviously 17:15 sfan5 i'd propose writing "indent split lines with 2 tabs" (as it's already done for conditionals) into our style guidelines 17:16 red-001 the whole code could use some cleanup and unittests 17:16 rubenwardy red-001: minetest or srp? 17:16 rubenwardy :) 17:16 red-001 yes 17:17 Krock ^ inclusive or 17:18 red-001 the tendency to cast irrlicht stuff generates a lot of warnings from address sanitizer 17:22 red-001 how important is s in SRP anyways? 17:22 rubenwardy loool 17:22 rubenwardy iSRP 17:22 rubenwardy by Minetest [inc] 17:23 red-001 we did kinda accidently make it easy to guess 17:25 paramat 2 tab indent is needed for conditionals, it's not needed elsewhere and just causes more line splitting 17:26 paramat i think indenting of long lines should be left to coder judgement, whatever makes it more readable 17:28 paramat in some situations readability is more imprtant than a hard rule, which is why linters inherently fail 17:42 srifqi PR 7486 seems to be working. 17:42 rubenwardy # please 17:42 rubenwardy #7486 17:42 ShadowBot https://github.com/minetest/minetest/issues/7486 -- Software inventorycube by numberZero 17:43 srifqi Okay. 17:46 paramat many android bugfixes coming in, please review to prepare for 5.0.0 :) 17:47 paramat btw, is anyone intending to work on securing builtin against CSM? it's a 5.0.0 blocker 17:54 paramat probably not, which is understandable :) nerz probably won't do it 19:08 p_gimeno isn't p[a] equiv to p + a*sizeof(*p) 19:08 p_gimeno ^ strictly speaking, no. It's actually equivalent to *(p + a) 19:11 p_gimeno where p + 1 is a pointer to the element next to the one p points to, i.e. pointer addition has the sizeof implied, sorta 20:14 Shara Will merge game#2155 and game#2156 in a bit 20:14 ShadowBot https://github.com/minetest/minetest_game/issues/2155 -- Add butterflies mod by Ezhh 20:14 ShadowBot https://github.com/minetest/minetest_game/issues/2156 -- Make hidden fireflies floodable by Ezhh 20:24 nerzhul hey :) 20:25 nerzhul the android fixes are waited in 0.4.17 too if they permit to fix that game loading problem :( 20:53 paramat good point 20:54 sfan5 #7482 fixes the crashes people have been experiencing 20:54 ShadowBot https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 20:57 nerzhul sfan5, approved 21:02 paramat affects all platforms, so do we need a 0.4.17.2 release? :) 21:03 Krock technically yes, but the only popular affected platform for us is Android 21:04 sfan5 honestly, wondering why nobody has reported the bug on e.g. rpi yet 21:04 sfan5 guess mt is too hard to get running on an rpi 21:04 Krock right, RPI is on ARM too 21:04 paramat ok 21:05 sfan5 there are two options: 1) whoever tested it used a compiler that wasn't smart enough to do this optimization 2) nobody has hit this bug 21:05 Krock Megaf used to run a server on the RPI but I'm not sure how up to date he is with the setup 21:05 p_gimeno 3) nobody has bothered to report it (happens quite often) 21:07 Krock 4) they hit the bug but didn't know why it was crashing (lack of developer tools, knowledge and interest to debug) 21:08 paramat hmm however, surely we need a release of MT 0.4.17.2 if some android bugfixes are now going to be added to the app? otherwise seems messy 21:27 red-001 can be an android only release 21:55 paramat yeah perhaps :) 23:06 paramat will merge #7482 in 5 mins 23:06 ShadowBot https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 23:10 paramat going to do a little re-ordering of lua_api.txt now so please don't merge any lua_api.txt PRs until tomorrow morning EU time =) 23:10 paramat merging 23:12 paramat done