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     <rubenwardy> 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